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

[Pedro Alves <palves@redhat.com>]

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