[Ada] Fix completion for multiple function matches
Commit Message
On 08/31/2015 07:25 PM, Joel Brobecker wrote:
> Overall, the patch looks like definite progress as is, and I'd like
> to see it pushed to avoid both the unsollicited MCQ and the ensuing
> SEGV. However, I'm wondering if it might still be leaving the hole
> in the completion.
>
> In particular, what happens to the completion of "p ambig" if
> you add a second function called, say "Aux_Pck.Ambiguous_Prog"?
> In other words, I am wondering if the fact that we're returning
> zero when parsing for completion might not unexpectedly filter out
> some matches. Regardless of the answer, that's probably a worthwhile
> adjustment to the tests you propose.
I adjusted the testcase in the way I think you meant: I added the
Ambiguous_Prog procedure to Aux_Pck and added a test to ask the
completion of "p ambig": GDB correctly reports two possible completions:
"p ambiguous_func" and "p ambiguous_proc". This looks independent of
function resolution in ada-lang, and that is fortunate in our case. :-)
Is this what you had in mind?
> So, here's the plan I propose: We adjust the very minor suggestions
> I made below, adjust the testcase as suggested above, do a quick
> second round of review and then push the patch. If if turns out
> there is a hole (I am not sure), then we can put that on the list
> of things to improve and decide when we want to look at it.
Sure: here's the updated patch, re-regtested on x86_64-linux.
>> + /* If we got multiple matches, ask the user which one to use. Don't do this
>> + interactive thing during completion, though: it is irritating to get a
>> + prompt in this context. */
>
> I wouldn't say "irritating" as, to me, this is a bug, not an
> inconvenience. So, I would change the comment to say something like:
>
> /* If we got multiple matches, ask the user which one to use. Don't do this
> interactive thing during completion, though, as the purpose of the
> completion is providing a list of all possible matches. Prompting the
> user to filter it down would be completely unexpected in this case. */
Done.
>> +# Usually, parsing a function name that is ambiguous yields a menu through
>> +# which users can select a specific function. This should not happen during
>> +# completion, though.
>
> Missing second space after a period.
Done.
>> +test_gdb_complete "ambig" \
>> + "p ambiguous_func"
>> +test_gdb_complete "ambiguous_func" \
>> + "p ambiguous_func"
>> diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.adb b/gdb/testsuite/gdb.ada/complete/aux_pck.adb
>> new file mode 100644
>> index 0000000..18e060e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.adb
>> @@ -0,0 +1,23 @@
>> +-- Copyright 2008-2015 Free Software Foundation, Inc.
>
> I *think* you want to only list 2015 in this case. Unless I missed
> the fact that you copied this file from elsewhere...
Copy-paste error, indeed. ;-) Done.
>> diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.ads b/gdb/testsuite/gdb.ada/complete/aux_pck.ads
>> new file mode 100644
>> index 0000000..36fdb6c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.ads
>> @@ -0,0 +1,20 @@
>> +-- Copyright 2008-2015 Free Software Foundation, Inc.
>
> Same here.
Likewise.
Thank you for reviewing!
Comments
> I adjusted the testcase in the way I think you meant: I added the
> Ambiguous_Prog procedure to Aux_Pck and added a test to ask the completion
> of "p ambig": GDB correctly reports two possible completions: "p
> ambiguous_func" and "p ambiguous_proc". This looks independent of function
> resolution in ada-lang, and that is fortunate in our case. :-)
>
> Is this what you had in mind?
Yep :)
> gdb/ChangeLog:
>
> * ada-lang.c (ada_resolve_function): Do not ask the user what
> match to use when in completion mode.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.ada/complete.exp: Add "pck.ambiguous_func" to the relevant
> expected outputs. Add two testcases for completing ambiguous
> functions.
> * gdb.ada/complete/aux_pck.adb: New file.
> * gdb.ada/complete/aux_pck.ads: New file.
> * gdb.ada/complete/foo.adb: Pull Aux_Pck and call the two
> Ambiguous_Func functions.
> * gdb.ada/complete/pck.ads: Add an Ambiguous_Func function.
> * gdb.ada/complete/pck.adb: Likewise.
Looks good. Go ahead and push :).
Thank you, Pierre-Marie.
On 09/01/2015 02:15 PM, Joel Brobecker wrote:
>> Is this what you had in mind?
>
> Yep :)
Cool!
> Looks good. Go ahead and push :).
>
> Thank you, Pierre-Marie.
This is pushed. Thank you very much!
From 1ce5e145bd6147507fcae4fb614b6cb4fdefd2bc Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Mon, 31 Aug 2015 16:04:07 +0200
Subject: [PATCH] [Ada] Fix completion for multiple function matches
Before this change, trying to complete an expression ending with an
ambiguous function name (i.e. for which there are multiple matches)
would display a menu with a prompt for the user to pick one. For
instance:
(gdb) p func<tab>Multiple matches for func
[0] cancel
[1] pack2.func at pack2.adb:5
[2] pack.func at pack.adb:5
>
This is not user friendly and actually triggered a segmentation fault
after the user did pick one. It is not clear whether the segmentation
fault needs a separate fix, but this is the only known case which
exhibits it at the moment, and this case must be fixed itself.
The problem lies in ada-lang.c (ada_resolve_function): when we got
multiple matches, we should not display the menu if we are in completion
mode. This patch adjusts the corresponding condition accordingly.
gdb/ChangeLog:
* ada-lang.c (ada_resolve_function): Do not ask the user what
match to use when in completion mode.
gdb/testsuite/ChangeLog:
* gdb.ada/complete.exp: Add "pck.ambiguous_func" to the relevant
expected outputs. Add two testcases for completing ambiguous
functions.
* gdb.ada/complete/aux_pck.adb: New file.
* gdb.ada/complete/aux_pck.ads: New file.
* gdb.ada/complete/foo.adb: Pull Aux_Pck and call the two
Ambiguous_Func functions.
* gdb.ada/complete/pck.ads: Add an Ambiguous_Func function.
* gdb.ada/complete/pck.adb: Likewise.
Tested on x86_64-linux, no regression.
---
gdb/ada-lang.c | 6 +++++-
gdb/testsuite/gdb.ada/complete.exp | 13 +++++++++++++
gdb/testsuite/gdb.ada/complete/aux_pck.adb | 28 ++++++++++++++++++++++++++++
gdb/testsuite/gdb.ada/complete/aux_pck.ads | 21 +++++++++++++++++++++
gdb/testsuite/gdb.ada/complete/foo.adb | 6 +++++-
gdb/testsuite/gdb.ada/complete/pck.adb | 5 +++++
gdb/testsuite/gdb.ada/complete/pck.ads | 2 ++
7 files changed, 79 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.ada/complete/aux_pck.adb
create mode 100644 gdb/testsuite/gdb.ada/complete/aux_pck.ads
@@ -3655,9 +3655,13 @@ ada_resolve_function (struct block_symbol syms[],
}
}
+ /* If we got multiple matches, ask the user which one to use. Don't do this
+ interactive thing during completion, though, as the purpose of the
+ completion is providing a list of all possible matches. Prompting the
+ user to filter it down would be completely unexpected in this case. */
if (m == 0)
return -1;
- else if (m > 1)
+ else if (m > 1 && !parse_completion)
{
printf_filtered (_("Multiple matches for %s\n"), name);
user_select_syms (syms, m, 1);
@@ -140,6 +140,7 @@ test_gdb_complete "external_ident" \
test_gdb_complete "pck" \
[multi_line "(p pck\\.ad\[sb\])?" \
"(p pck\\.ad\[sb\])?" \
+ "p pck.ambiguous_func" \
"p pck.external_identical_one" \
"p pck.inner.inside_variable" \
"p pck.local_identical_one" \
@@ -151,6 +152,7 @@ test_gdb_complete "pck" \
test_gdb_complete "pck." \
[multi_line "(p pck\\.ad\[sb\])?" \
"(p pck\\.ad\[sb\])?" \
+ "p pck.ambiguous_func" \
"p pck.external_identical_one" \
"p pck.inner.inside_variable" \
"p pck.local_identical_one" \
@@ -181,3 +183,14 @@ if { [readline_is_used] } {
}
}
}
+
+# Usually, parsing a function name that is ambiguous yields a menu through
+# which users can select a specific function. This should not happen during
+# completion, though.
+test_gdb_complete "ambig" \
+ [multi_line "p ambiguous_func" \
+ "p ambiguous_proc" ]
+test_gdb_complete "ambiguous_f" \
+ "p ambiguous_func"
+test_gdb_complete "ambiguous_func" \
+ "p ambiguous_func"
new file mode 100644
@@ -0,0 +1,28 @@
+-- Copyright 2015 Free Software Foundation, Inc.
+--
+-- This program is free software; you can redistribute it and/or modify
+-- it under the terms of the GNU General Public License as published by
+-- the Free Software Foundation; either version 3 of the License, or
+-- (at your option) any later version.
+--
+-- This program is distributed in the hope that it will be useful,
+-- but WITHOUT ANY WARRANTY; without even the implied warranty of
+-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+-- GNU General Public License for more details.
+--
+-- You should have received a copy of the GNU General Public License
+-- along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package body Aux_Pck is
+
+ procedure Ambiguous_Func is
+ begin
+ null;
+ end Ambiguous_Func;
+
+ procedure Ambiguous_Proc is
+ begin
+ null;
+ end Ambiguous_Proc;
+
+end Aux_Pck;
new file mode 100644
@@ -0,0 +1,21 @@
+-- Copyright 2015 Free Software Foundation, Inc.
+--
+-- This program is free software; you can redistribute it and/or modify
+-- it under the terms of the GNU General Public License as published by
+-- the Free Software Foundation; either version 3 of the License, or
+-- (at your option) any later version.
+--
+-- This program is distributed in the hope that it will be useful,
+-- but WITHOUT ANY WARRANTY; without even the implied warranty of
+-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+-- GNU General Public License for more details.
+--
+-- You should have received a copy of the GNU General Public License
+-- along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package Aux_Pck is
+
+ procedure Ambiguous_Func;
+ procedure Ambiguous_Proc;
+
+end Aux_Pck;
@@ -13,7 +13,8 @@
-- You should have received a copy of the GNU General Public License
-- along with this program. If not, see <http://www.gnu.org/licenses/>.
-with Pck; use Pck;
+with Aux_Pck; use Aux_Pck;
+with Pck; use Pck;
procedure Foo is
Some_Local_Variable : Integer := 1;
@@ -21,5 +22,8 @@ procedure Foo is
begin
My_Global_Variable := Some_Local_Variable + 1; -- START
Proc (External_Identical_Two);
+ Aux_Pck.Ambiguous_Func;
+ Aux_Pck.Ambiguous_Proc;
+ Pck.Ambiguous_Func;
end Foo;
@@ -21,4 +21,9 @@ package body Pck is
Inner.Inside_Variable := Not_In_Scope + I;
end Proc;
+ procedure Ambiguous_Func is
+ begin
+ null;
+ end Ambiguous_Func;
+
end Pck;
@@ -31,4 +31,6 @@ package Pck is
procedure Proc (I : Integer);
+ procedure Ambiguous_Func;
+
end Pck;
--
2.5.1