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!

-- 
Pierre-Marie de Rodat