Re: [PATCH v3 2/2] MI: Add new command -complete

Pedro Alves
2019-04-03T19:23:35Z

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, &quote_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 <jan.vrany@fit.cvut.cz>
> +
> + * gdb.mi/mi-complete.exp: New file.
> + * gdb.mi/mi-complete.cc: Likewise.
> +
> 2019-01-21 Alan Hayward <alan.hayward@arm.com>
> * 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 <vector>

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<int> 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 <http://www.gnu.org/licenses/>.
> +
> +# 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