From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8694 invoked by alias); 31 Aug 2015 17:25:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 8684 invoked by uid 89); 31 Aug 2015 17:25:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 31 Aug 2015 17:25:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3253D291CE; Mon, 31 Aug 2015 13:25:18 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id ePjnSHmROBB4; Mon, 31 Aug 2015 13:25:18 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E8C63291C9; Mon, 31 Aug 2015 13:25:17 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 82838472DA; Mon, 31 Aug 2015 10:25:16 -0700 (PDT) Date: Mon, 31 Aug 2015 17:25:00 -0000 From: Joel Brobecker To: Pierre-Marie de Rodat Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [Ada] Fix completion for multiple function matches Message-ID: <20150831172516.GA3356@adacore.com> References: <1441034710-31810-1-git-send-email-derodat@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1441034710-31810-1-git-send-email-derodat@adacore.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-08/txt/msg00870.txt.bz2 > 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. Thanks for the patch, Pierre-Marie. 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. 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. > + /* 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. */ > +# 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. > +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... > 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. -- Joel