Re: [PATCH v3 02/19] Remove completion_tracker_t from the public completion API.

[Doug Evans <xdje42 at gmail dot com>]

Doug Evans <xdje42@gmail.com> writes:
> Keith Seitz <keiths@redhat.com> writes:
>>...
>> gdb/ChangeLog
>>
>> 	PR gdb/17960
>> 	* completer.c (struct completer_data) <tracker>: Change type to
>> 	htab_t.
>> 	(enum maybe_add_completion_enum): Make private; moved from
>> 	completer.h.
>> 	[DEFAULT_MAX_COMPLETIONS]: Define.
>> 	(new_completion_tracker): Remove function.
>> 	(new_completer_data): New function.
>> 	(free_completion_tracker): Remove function.
>> 	(free_completer_data): New function.
>> 	(make_cleanup_free_completion_tracker): Remove function.
>> 	(maybe_add_completion): Make static.  Update comments.
>> 	Use completer_data instead of completion_tracker_t.
>> 	(completer_strdup): New function.
>> 	(add_completion): New function.
>> 	(complete_line, gdb_completion_word_break_characters): Use
>> 	completer_data instead of completion_tracker.
>> 	* completer.h (completion_tracker_t, new_completion_tracker)
>> 	(make_cleanup_free_completion_tracker, maybe_add_completion)
>> 	(enum maybe_add_completion): Remove declarations.
>> 	(enum add_completion_status): Define.
>> 	(add_completion): Declare.
>> 	* symtab.c (completion_tracker): Remove variable.
>> 	(completion_list_add_name): Use add_completion instead of
>> 	maybe_add_completion and partial copy.
>> 	(default_make_symbol_completion_list_break_on_1): Do not use
>> 	completion_tracker.
>>
>> gdb/testsuite/ChangeLog
>>
>> 	PR gdb/17960
>> 	* gdb.base/completion.exp: Add some basic tests for the
>> 	location completer, including a regression test for
>> 	the gdb/17960 assertion failure.
>>...
>> @@ -803,48 +824,41 @@ complete_line_internal (struct completer_data *cdata,
>>  
>>  /* See completer.h.  */
>>  
>> -int max_completions = 200;
>> +#define DEFAULT_MAX_COMPLETIONS 200
>> +int max_completions = DEFAULT_MAX_COMPLETIONS;
>>  
>> -/* See completer.h.  */
>> +/* Allocate a new completer data structure.  */
>>  
>> -completion_tracker_t
>> -new_completion_tracker (void)
>> +static struct completer_data *
>> +new_completer_data (int size)
>>  {
>> -  if (max_completions <= 0)
>> -    return NULL;
>> +  struct completer_data *cdata = XCNEW (struct completer_data);
>>  
>> -  return htab_create_alloc (max_completions,
>> -			    htab_hash_string, (htab_eq) streq,
>> -			    NULL, xcalloc, xfree);
>> +  cdata->tracker
>> +    = htab_create_alloc ((size < 0 ? DEFAULT_MAX_COMPLETIONS : size),
>> +			 htab_hash_string, (htab_eq) streq, NULL,
>
> ====
>...
> I'm also slightly ambivalent on creating the hash table if
> max_completions is disabled (<= 0), and leaning towards not creating it
> to avoid potential confusion.
> Let's do the latter (pending reading the rest of the patchset - maybe
> there's a good reason to always create it).

Coming back to this now that I've gone through the entire series.
I now understand why the hashtable is always created. :-)
-> because it's now the only repository of completions.
So that's ok.

As I read the code the question "What does htab_create_alloc do if I
pass it a size of zero?" comes to mind.
It'll just start with a table size of the first prime it uses.
So that's ok.

I then checked the callers of new_completer_data:

complete_line does this:

  if (max_completions == 0)
    return NULL;

but gdb_completion_word_break_characters doesn't have this check
so new_completer_data has to handle any value for max_completions.

I don't think there's a problem here
(IIUC gdb_completion_word_break_characters correctly).
At the least it would be good to specify in the function comment of
new_completer_data that any value for size (-ve, zero, +ve) is ok.