From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21845 invoked by alias); 3 Apr 2019 19:23: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 21836 invoked by uid 89); 3 Apr 2019 19:23:35 -0000 Authentication-Results: sourceware.org; auth=none 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,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=channels, mq, mai, Hit X-HELO: mail-wm1-f44.google.com Received: from mail-wm1-f44.google.com (HELO mail-wm1-f44.google.com) (209.85.128.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 Apr 2019 19:23:33 +0000 Received: by mail-wm1-f44.google.com with SMTP id q16so102327wmj.3 for ; Wed, 03 Apr 2019 12:23:33 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id o10sm21478670wru.54.2019.04.03.12.23.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Apr 2019 12:23:30 -0700 (PDT) Subject: Re: [PATCH v3 2/2] MI: Add new command -complete To: Jan Vrany , gdb-patches@sourceware.org References: <20190128124101.26243-1-jan.vrany@fit.cvut.cz> <20190304145203.11039-3-jan.vrany@fit.cvut.cz> From: Pedro Alves Message-ID: Date: Wed, 03 Apr 2019 19:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190304145203.11039-3-jan.vrany@fit.cvut.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-04/txt/msg00058.txt.bz2 Hi Jan, This version generally looks good to me. There are a few nits to address, but with those picked, this should be good to go. On 03/04/2019 02:52 PM, Jan Vrany wrote: > diff --git a/gdb/NEWS b/gdb/NEWS > index eaef6aa384..3018313a46 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -96,6 +96,13 @@ maint show dwarf unwinders > info proc files > Display a list of open files for a process. > > +* New MI commands > + > +-complete > + This lists all the possible completions for the rest of the line, if it > + were to be given as a command itself. This is intended for use by MI frontends > + in cases when separate CLI and MI channels cannot be used. That line looks too long. Hit M-q in emacs to reflow it. > --- a/gdb/mi/mi-cmds.c > +++ b/gdb/mi/mi-cmds.c > @@ -23,6 +23,7 @@ > #include "mi-cmds.h" > #include "mi-main.h" > > + Spurious space here. Please drop it. > struct mi_cmd; > static struct mi_cmd **lookup_table (const char *command); > static void build_table (struct mi_cmd *commands); > @@ -75,6 +76,7 @@ static struct mi_cmd mi_cmds[] = > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -2709,6 +2709,55 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc) > } > } > > +/* Implement the "-complete" command. */ > + > +void > +mi_cmd_complete (const char *command, char **argv, int argc) > +{ > + if (argc != 1) > + { > + error (_("Usage: -complete COMMAND")); > + } We don't wrap single-line statements with {}'s. > + if (max_completions == 0) > + { > + error (_("max-completions is zero," > + " completion is disabled.\n")); > + } > + > + int quote_char = '\0'; > + const char *word; > + > + completion_result result = complete (argv[0], &word, "e_char); > + > + std::string arg_prefix (argv[0], word - argv[0]); > + > + struct ui_out *uiout = current_uiout; > + > + if (result.number_matches > 0) > + uiout->field_fmt ("completion", "%s%s", arg_prefix.c_str (), result.match_list[0]); > + > + { > + ui_out_emit_list completions_emitter (uiout, "matches"); > + > + if (result.number_matches == 1) > + { > + uiout->field_fmt(NULL, "%s%s", arg_prefix.c_str (), result.match_list[0]); > + } Ditto. Missing space before parens in "field_fmt(NULL". Watch out for too-long lines -- 80 cols is the hard max. > + else > + { > + result.sort_match_list (); > + for (size_t i = 0; i < result.number_matches; i++) > + { > + uiout->field_fmt(NULL, "%s%s", arg_prefix.c_str (), > + result.match_list[i + 1]); Missing space before parens in "field_fmt(NULL". > + } > + } > + } > + uiout->field_string ("max_completions_reached", > + result.number_matches == max_completions ? "1" : "0"); > +} > + > + > void > _initialize_mi_main (void) > { > diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog > index 7d8c7908fe..03135837f8 100644 > --- a/gdb/testsuite/ChangeLog > +++ b/gdb/testsuite/ChangeLog > @@ -1,3 +1,8 @@ > +2019-01-28 Jan Vrany > + > + * gdb.mi/mi-complete.exp: New file. > + * gdb.mi/mi-complete.cc: Likewise. > + > 2019-01-21 Alan Hayward > * gdb.base/infcall-nested-structs.exp: Test C++ in addition to C. > > diff --git a/gdb/testsuite/gdb.mi/mi-complete.cc b/gdb/testsuite/gdb.mi/mi-complete.cc > new file mode 100644 > index 0000000000..fc85057d69 > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-complete.cc > @@ -0,0 +1,21 @@ > +#include Missing copyright header. > + > +class A > +{ > +public: > + void push_back(void *value); > +}; > + > +void A::push_back(void *value) > +{ > + /* nothing */ > +} > + > +int main(int argc, char **argv) > +{ > + std::vector v; > + v.push_back(1); > + A a; > + a.push_back(&v); > + return 0; > +} Please adjust the formatting to follow GNU conventions. > \ No newline at end of file Please add the missing newline. > diff --git a/gdb/testsuite/gdb.mi/mi-complete.exp b/gdb/testsuite/gdb.mi/mi-complete.exp > new file mode 100644 > index 0000000000..e82c2bff40 > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-complete.exp > @@ -0,0 +1,75 @@ > +# Copyright 2018 Free Software Foundation, Inc. This needs to be 2018-2019 now. > + > +# 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 . > + > +# Verify -data-evaluate-expression. There are really minimal tests. Please replace this with a description of what this testcase is about. > + > +# The goal is not to test gdb functionality, which is done by other tests, > +# but to verify the correct output response to MI operations. > +# Drop this. > + > +load_lib mi-support.exp > +set MIFLAGS "-i=mi" > + > +gdb_exit > +if [mi_gdb_start] { > + continue > +} > + > +standard_testfile .cc > + > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } { Spurious double space after "if". > + untested "failed to compile" > + return -1 > +} > + > +mi_run_to_main > + > +mi_gdb_test "1-complete br" \ > + "1\\^done,completion=\"break\",matches=\\\[.*\"break\",.*\"break-range\".*\\\],max_completions_reached=\"0\"" \ > + "-complete br" > + > +# Check empty completion list Write complete sentences -- add the missing period. > +mi_gdb_test "5-complete bogus" \ > + "5\\^done,matches=\\\[\\\],max_completions_reached=\"0\"" \ > + "-complete bogus" > + > +# Check completions for commands with space Missing period. > +mi_gdb_test "4-complete \"b mai\"" \ > + "4\\^done,completion=\"b main\",matches=\\\[.*\"b main\".*\\\],max_completions_reached=\"0\"" \ > + "-complete \"b mai\"" > + > +# Check wildmatching Missing period. > +mi_gdb_test "5-complete \"b push_ba\"" \ > + "5\\^done,completion=\"b push_back\\(\",matches=\\\[.*\"b A::push_back\\(void\\*\\)\".*\\\],max_completions_reached=\"0\"" \ > + "-complete \"b push_ba\" (wildmatching)" Please see: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook?highlight=%28testcase%29#Do_not_use_.22tail_parentheses.22_on_test_messages > + > +mi_gdb_test "-info-gdb-mi-command complete" \ > + "\\^done,command=\{exists=\"true\"\}" \ > + "-info-gdb-mi-command complete" > + > +# Limit max completions and check that max_completions_reached=\"0\" is set > +# to 1. > +send_gdb "set max-completions 1\n" > + > +mi_gdb_test "2-complete br" \ > + ".*2\\^done,completion=\"br\[A-Za-z0-9-\]+\",matches=\\\[\"br\[A-Za-z0-9-\]+\"\\\],max_completions_reached=\"1\"" \ > + "-complete br (max-completions 1)" > + > +# Disable completions and check an error is returned > +send_gdb "set max-completions 0\n" > + > +mi_gdb_test "3-complete br" \ > + ".*3\\^error,msg=\".*" \ > + "-complete br (max-completions 0)" > Thanks, Pedro Alves