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

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

Thanks,
Pedro Alves

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

Commit Message

Comments

Patch