From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71353 invoked by alias); 15 Jul 2017 00:07:35 -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 71341 invoked by uid 89); 15 Jul 2017 00:07:34 -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=NOTHING, evil, believes, advances 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; Sat, 15 Jul 2017 00:07:32 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8ED787F403 for ; Sat, 15 Jul 2017 00:07:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8ED787F403 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=keiths@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 8ED787F403 Received: from valrhona.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3B71E183AB; Sat, 15 Jul 2017 00:07:31 +0000 (UTC) Subject: Re: [PATCH 18/40] A smarter linespec completer To: Pedro Alves , gdb-patches@sourceware.org References: <1496406158-12663-1-git-send-email-palves@redhat.com> <1496406158-12663-19-git-send-email-palves@redhat.com> From: Keith Seitz Message-ID: <59695CC2.2090206@redhat.com> Date: Sat, 15 Jul 2017 00:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1496406158-12663-19-git-send-email-palves@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00207.txt.bz2 This is awesome. Just some minor nits. On 06/02/2017 05:22 AM, Pedro Alves wrote: > gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves > > * gdb.linespec/ls-errs.exp: Don't sent tab characters, now that > the completer works. Typo "sent" > diff --git a/gdb/completer.h b/gdb/completer.h > index eab9c69..fbfe4d5 100644 > --- a/gdb/completer.h > +++ b/gdb/completer.h > @@ -255,6 +265,14 @@ private: > completable words. */ > int m_custom_word_point = 0; > > + /* If true, tell readline to skip appending a whitespace after the > + completion. Automatically set if we have a unique completion > + that already has a space at the end. Completer may also > + explicitly set this. E.g., the linespec completer sets when when Typos: "... completer sets [this] when *when* .." > + the completion ends with the ":" separator between filename and > + function name. */ > + bool m_suppress_append_ws = false; > + > /* Our idea of lowest common denominator to hand over to readline. > See intro. */ > char *m_lowest_common_denominator = NULL; > @@ -347,6 +365,11 @@ extern completer_handle_brkchars_ftype * > > /* Exported to linespec.c */ > > +extern completion_list complete_source_filenames (const char *text); > + > +extern void complete_expression (completion_tracker &tracker, > + const char *text, const char *word); > + > extern const char *skip_quoted_chars (const char *, const char *, > const char *); Should the explanatory comments in completer.c be moved here? > diff --git a/gdb/linespec.c b/gdb/linespec.c > index f24cca2..c993c67 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -271,6 +293,29 @@ struct ls_parser > /* The result of the parse. */ > struct linespec result; > #define PARSER_RESULT(PPTR) (&(PPTR)->result) > + > + /* What the parser believes the current word point should complete > + to. */ > + linespec_complete_what complete_what; > + > + /* The completion word point. The parser advances this as is skips Typo "as i[t] skips" > + tokens. At some point the input string will end or parsing will > + fail, and then we attempt completion at the captured completion > + word point, interpreting the string at completion_word as > + COMPLETE_WHAT. */ > + const char *completion_word; > + > + /* If the current token was a quoted string, then this is the > + quoting character (either " or '.). */ Typo {either " or '.).} (extra period inside parenthesis) > + int completion_quote_char; Why int? > @@ -543,6 +588,30 @@ find_parameter_list_end (const char *input) > return p; > } > > +/* If the [STRING, STRING_LEN) string ends with what looks like a > + keyword, return the keyword start offset in STRING. Return -1 > + otherwise. */ > + > +size_t > +string_find_incomplete_keyword_at_end (const char * const *keywords, > + const char *string, size_t string_len) Should this be static? > + else if (component == linespec_complete_what::FUNCTION) > + { > + completion_list fn_list; > + > + linespec_complete_function (tracker, text, source_filename); > + if (source_filename == NULL) > + fn_list = complete_source_filenames (text); > + Maybe I took the description of FUNCTION too literally, but it was not obvious to me why we search for source files here. When parse_linespec returns (after, e.g., "break foo"), PARSER_EXPLICIT->function_name == "foo", but here we search for both functions *and* source files starting with "foo". Given the ambiguity in the grammar, this is a necessary evil. The meaning of FUNCTION is overloaded (it is not just "functions/methods"). Can a comment be added to clarify this (probably at linespec_complete_what::FUNCTION)? > + else if (parser.complete_what == linespec_complete_what::FUNCTION) > + { > + /* While parsing/lexing, we didn't know whether the completion > + word completes to a unique function name already or not. E.g.: Similarly here, I would like to see "unique function or source file name". A casual reader may really easily overlook this. > + "b function() " > + may need to complete either to: > + "b function() const" > + or to: > + "b function() if/thread/task" > + > + Or, this: > + "b foo t" > + may need to complete either to: > + "b foo template_fun()" > + with "foo" being the template function's return type, or to: > + "b foo thread/task" > + > + Address that by completing assuming function, and seeing if > + we find a function completion that matches exactly > + "function()". If so, then we advance the completion word past > + the function and switch to KEYWORD completion mode. */ > + > + const char *text = parser.completion_word; > + const char *word = parser.completion_word; > + > + complete_linespec_component (&parser, tracker, > + parser.completion_word, > + linespec_complete_what::FUNCTION, > + PARSER_EXPLICIT (&parser)->source_filename); > + > + parser.complete_what = linespec_complete_what::NOTHING; > + > + if (tracker.quote_char ()) > + { > + /* The function/file name was not close-quoted, so this > + can't be a keyword. */ quote_char could be ':' (from complete_linespec_component). Please add a comment reminding the reader. [Well, that could easily be me, and I'm gettin' old!] Keith