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

[Keith Seitz <keiths@redhat.com>]

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.

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.

> +
> +/* 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"?

> +
> +/* 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"?

> 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...").

>  
> -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/ ?

> -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...").

Keith