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

Doug Evans
2015-08-24T16:06:49Z

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.