From patchwork Mon Jul 17 13:56:13 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 21644 Received: (qmail 9087 invoked by alias); 17 Jul 2017 13:56:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 8005 invoked by uid 89); 17 Jul 2017 13:56:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=yup X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Jul 2017 13:56:17 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F41C480E72 for ; Mon, 17 Jul 2017 13:56:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F41C480E72 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com F41C480E72 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C5C7E5D6A2; Mon, 17 Jul 2017 13:56:14 +0000 (UTC) Subject: Re: [PATCH 11/40] Introduce class completion_tracker & rewrite completion<->readline interaction To: Keith Seitz , gdb-patches@sourceware.org References: <1496406158-12663-1-git-send-email-palves@redhat.com> <1496406158-12663-12-git-send-email-palves@redhat.com> <5968FDFE.80302@redhat.com> From: Pedro Alves Message-ID: <560f7c11-e627-6cc6-7b57-fa300dd6f3c2@redhat.com> Date: Mon, 17 Jul 2017 14:56:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <5968FDFE.80302@redhat.com> On 07/14/2017 06:23 PM, Keith Seitz wrote: > On 06/02/2017 05:22 AM, Pedro Alves wrote: > >> Adds a new "completion_tracker" class that is meant to hold everything >> about the state of the current completion operation. > > This is quite close to the approach I attempted in the series I submitted (cough) in 2015. Yeah, I'm sorry about that. As you know, I wasn't involved in that review back then, and I had assumed master already contained all the patches you had had back then... After you pointed me at them, I considered rebasing on top of them. But since your patches (naturally) were still using VEC (since they predated C++), we'd end touching/redoing the same exact same code throughout twice, to work with the completion_tracker_t methods. :-/ > > Just have a few minor comments (about comments!). > >> diff --git a/gdb/completer.c b/gdb/completer.c >> index fe69faa..c6e1e28 100644 >> --- a/gdb/completer.c >> +++ b/gdb/completer.c >> @@ -429,10 +420,10 @@ backup_text_ptr (const char *p, const char *text) >> /* A completer function for explicit locations. This function >> completes both options ("-source", "-line", etc) and values. */ >> >> -static VEC (char_ptr) * >> -explicit_location_completer (struct cmd_list_element *ignore, >> - struct event_location *location, >> - const char *text, const char *word) >> +static void >> +complete_explicit_location (completion_tracker &tracker, >> + struct event_location *location, >> + const char *text, const char *word) >> { >> const char *p; >> VEC (char_ptr) *matches = NULL; > > `matches' is no longer used. [I realize this will disappear in a later patch.] > >> /* See completer.h. */ >> >> -enum maybe_add_completion_enum >> -maybe_add_completion (completion_tracker_t tracker, char *name) >> +bool >> +completion_tracker::maybe_add_completion (gdb::unique_xmalloc_ptr name) >> { > [snip] > >> >> - return (htab_elements (tracker) < max_completions >> - ? MAYBE_ADD_COMPLETION_OK >> - : MAYBE_ADD_COMPLETION_OK_MAX_REACHED); >> +void >> +completion_tracker::add_completion (gdb::unique_xmalloc_ptr name) >> +{ >> + if (!maybe_add_completion (std::move (name))) >> + throw_max_completions_reached_error (); >> } >> >> void >> @@ -1075,10 +1075,16 @@ throw_max_completions_reached_error (void) >> throw_error (MAX_COMPLETIONS_REACHED_ERROR, _("Max completions reached.")); >> } >> >> -/* Generate completions all at once. Returns a vector of unique strings >> - allocated with xmalloc. Returns NULL if there are no completions >> - or if max_completions is 0. If max_completions is non-negative, this will >> - return at most max_completions strings. >> +void >> +completion_tracker::add_completions (completion_list &&list) >> +{ >> + for (auto &candidate : list) >> + add_completion (std::move (candidate)); >> +} > > Some of the above methods have comments (per convention, "See XYZ.h"), some do not. [There are a bunch more methods with no comments below this, too.] > > Is this requirement being relaxed for C++? I certainly wouldn't mind if we started assuming "See XYZ.h" for all methods, but I don't think convention has been discussed/codified yet. Indeed, I don't think it's been discussed. For now, I added the comments, but I wouldn't mind relaxing either. > >> + >> +/* Build a new C string that is a copy or LCD with the whitespace of >> + ORIG/ORIG_LEN preserved. > > Is this supposed to be "a copy *of* LCD"? Indeed. an "inline line" in "we want to end up with an input line like" was supposed to be "input line". Gah. > >> + >> +/* Helper for gdb_rl_attempted_completion_function, which does most of >> + the work. This is called by readline to build the match list >> + array, and determining the lowest common denominator. The real > > This last sentence isn't right. At its simplest, it says, "This is called, and determining the lowest common denominator." Is that supposed to be, "This is called to build.. and to determine"? Yup, thanks. > >> diff --git a/gdb/symtab.c b/gdb/symtab.c >> index 09c9411b..cd78a16 100644 >> --- a/gdb/symtab.c >> +++ b/gdb/symtab.c >> >> /* Return a vector of all symbols (regardless of class) which begin by >> matching TEXT. If the answer is no symbols, then the return value >> is NULL. */ > > This comment needs updating ("Return a vector..."). Did that now. > >> >> -VEC (char_ptr) * >> -make_symbol_completion_list (const char *text, const char *word) >> +void >> +collect_symbol_completion_matches (completion_tracker &tracker, >> + const char *text, const char *word) >> { >> - return current_language->la_make_symbol_completion_list (text, word, >> - TYPE_CODE_UNDEF); >> + current_language->la_collect_symbol_completion_matches (tracker, >> + text, word, >> + TYPE_CODE_UNDEF); >> } >> >> -/* Like make_symbol_completion_list, but only return STRUCT_DOMAIN >> - symbols whose type code is CODE. */ >> +/* Like collect_symbol_completion_matches, but only return >> + STRUCT_DOMAIN symbols whose type code is CODE. */ >> > > s/return/collect/ ? Fixed. > >> -VEC (char_ptr) * >> -make_symbol_completion_type (const char *text, const char *word, >> - enum type_code code) >> +void >> +collect_symbol_completion_matches_type (completion_tracker &tracker, >> + const char *text, const char *word, >> + enum type_code code) >> { >> gdb_assert (code == TYPE_CODE_UNION >> || code == TYPE_CODE_STRUCT >> || code == TYPE_CODE_ENUM); >> - return current_language->la_make_symbol_completion_list (text, word, code); >> + current_language->la_collect_symbol_completion_matches (tracker, >> + text, word, code); >> } >> >> @@ -5503,17 +5419,16 @@ maybe_add_partial_symtab_filename (const char *filename, const char *fullname, >> >> /* Return a vector of all source files whose names begin with matching >> TEXT. The file names are looked up in the symbol tables of this >> - program. If the answer is no matchess, then the return value is >> - NULL. */ >> + program. */ > > This comment also needs updating ("Return a vector..."). > Thanks much Keith. I pushed the patch with the below squashed in. (While adding the missing comments I noticed that throw_max_completions_reached_error could be static, and then since there's only one caller, I inlined it.) Thanks, Pedro Alves diff --git i/gdb/completer.c w/gdb/completer.c index c6e1e28..85e6d88 100644 --- i/gdb/completer.c +++ w/gdb/completer.c @@ -426,7 +426,6 @@ complete_explicit_location (completion_tracker &tracker, const char *text, const char *word) { const char *p; - VEC (char_ptr) *matches = NULL; /* Find the beginning of the word. This is necessary because we need to know if we are completing an option name or value. We @@ -1029,6 +1028,8 @@ completion_tracker::completion_tracker () NULL, xcalloc, xfree); } +/* See completer.h. */ + completion_tracker::~completion_tracker () { xfree (m_lowest_common_denominator); @@ -1062,18 +1063,16 @@ completion_tracker::maybe_add_completion (gdb::unique_xmalloc_ptr name) return true; } +/* See completer.h. */ + void completion_tracker::add_completion (gdb::unique_xmalloc_ptr name) { if (!maybe_add_completion (std::move (name))) - throw_max_completions_reached_error (); + throw_error (MAX_COMPLETIONS_REACHED_ERROR, _("Max completions reached.")); } -void -throw_max_completions_reached_error (void) -{ - throw_error (MAX_COMPLETIONS_REACHED_ERROR, _("Max completions reached.")); -} +/* See completer.h. */ void completion_tracker::add_completions (completion_list &&list) @@ -1337,7 +1336,7 @@ completion_tracker::recompute_lowest_common_denominator (const char *new_match) } } -/* Build a new C string that is a copy or LCD with the whitespace of +/* Build a new C string that is a copy of LCD with the whitespace of ORIG/ORIG_LEN preserved. Say the user is completing a symbol name, with spaces, like: @@ -1348,7 +1347,7 @@ completion_tracker::recompute_lowest_common_denominator (const char *new_match) "foo(int)" - we want to end up with an inline line like: + we want to end up with an input line like: "foo ( int)" ^^^^^^^ => text from LCD [1], whitespace from ORIG preserved. @@ -1402,6 +1401,8 @@ expand_preserving_ws (const char *orig, size_t orig_len, return xstrdup (res.c_str ()); } +/* See completer.h. */ + completion_result completion_tracker::build_completion_result (const char *text, int start, int end) @@ -1443,11 +1444,15 @@ completion_tracker::build_completion_result (const char *text, } } +/* See completer.h */ + completion_result::completion_result () : match_list (NULL), number_matches (0), completion_suppress_append (false) {} +/* See completer.h */ + completion_result::completion_result (char **match_list_, size_t number_matches_, bool completion_suppress_append_) @@ -1456,11 +1461,15 @@ completion_result::completion_result (char **match_list_, completion_suppress_append (completion_suppress_append_) {} +/* See completer.h */ + completion_result::~completion_result () { reset_match_list (); } +/* See completer.h */ + completion_result::completion_result (completion_result &&rhs) { if (this == &rhs) @@ -1473,6 +1482,8 @@ completion_result::completion_result (completion_result &&rhs) rhs.number_matches = 0; } +/* See completer.h */ + char ** completion_result::release_match_list () { @@ -1489,6 +1500,8 @@ compare_cstrings (const char *str1, const char *str2) return strcmp (str1, str2) < 0; } +/* See completer.h */ + void completion_result::sort_match_list () { @@ -1502,6 +1515,8 @@ completion_result::sort_match_list () } } +/* See completer.h */ + void completion_result::reset_match_list () { @@ -1515,9 +1530,9 @@ completion_result::reset_match_list () } /* Helper for gdb_rl_attempted_completion_function, which does most of - the work. This is called by readline to build the match list - array, and determining the lowest common denominator. The real - matches list starts at match[1], while match[0] is the slot holding + the work. This is called by readline to build the match list array + and to determine the lowest common denominator. The real matches + list starts at match[1], while match[0] is the slot holding readline's idea of the lowest common denominator of all matches, which is what readline replaces the completion "word" with. diff --git i/gdb/completer.h w/gdb/completer.h index e554bff..4b3b188 100644 --- i/gdb/completer.h +++ w/gdb/completer.h @@ -274,9 +274,4 @@ extern const char *skip_quoted (const char *); extern int max_completions; - -/* Wrapper to throw MAX_COMPLETIONS_REACHED_ERROR. */ - -extern void throw_max_completions_reached_error (void); - #endif /* defined (COMPLETER_H) */ diff --git i/gdb/symtab.c w/gdb/symtab.c index b70a818..57fb355 100644 --- i/gdb/symtab.c +++ w/gdb/symtab.c @@ -5217,9 +5217,8 @@ default_collect_symbol_completion_matches (completion_tracker &tracker, code); } -/* Return a vector of all symbols (regardless of class) which begin by - matching TEXT. If the answer is no symbols, then the return value - is NULL. */ +/* Collect all symbols (regardless of class) which begin by matching + TEXT. */ void collect_symbol_completion_matches (completion_tracker &tracker, @@ -5230,7 +5229,7 @@ collect_symbol_completion_matches (completion_tracker &tracker, TYPE_CODE_UNDEF); } -/* Like collect_symbol_completion_matches, but only return +/* Like collect_symbol_completion_matches, but only collect STRUCT_DOMAIN symbols whose type code is CODE. */ void @@ -5406,7 +5405,7 @@ maybe_add_partial_symtab_filename (const char *filename, const char *fullname, } } -/* Return a vector of all source files whose names begin with matching +/* Return a list of all source files whose names begin with matching TEXT. The file names are looked up in the symbol tables of this program. */