Re: [PATCH 11/40] Introduce class completion_tracker & rewrite completion<->readline interaction

[Christophe Lyon <christophe.lyon@st.com>]

On 17/07/2017 15:56, Pedro Alves wrote:
> 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<char> 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<char> 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.)

Hi Pedro,

Since you committed this patch, I've noticed that gdb fails to builds for some targets, such as arm-none-eabi (build is OK for arm-linux-gnueabi)

The build log says:

../../../gdb/remote-sim.c: In function 'void _initialize_remote_sim()':
../../../gdb/remote-sim.c:1350:46: error: invalid conversion from 'VEC_char_ptr* (*)(cmd_list_element*, const char*, const char*)' to 'void (*)(cmd_list_element*, completion_tracker&, const char*, const char*)' [-fpermissive]
    set_cmd_completer (c, sim_command_completer);
                                               ^
In file included from ../../../gdb/completer.h:21:0,
                  from ../../../gdb/symtab.h:28,
                  from ../../../gdb/language.h:26,
                  from ../../../gdb/frame.h:72,
                  from ../../../gdb/gdbarch.h:39,
                  from ../../../gdb/defs.h:636,
                  from ../../../gdb/remote-sim.c:23:
../../../gdb/command.h:197:13: error:   initializing argument 2 of 'void set_cmd_completer(cmd_list_element*, void (*)(cmd_list_element*, completion_tracker&, const char*, const char*))' [-fpermissive]
  extern void set_cmd_completer (struct cmd_list_element *, completer_ftype *);
make[2]: *** [remote-sim.o] Error 1

Can you have a look?

Thanks

> 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<char> name)
>     return true;
>   }
>   
> +/* See completer.h.  */
> +
>   void
>   completion_tracker::add_completion (gdb::unique_xmalloc_ptr<char> 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.  */
>   
> .
>