Re: [PATCH v3 18/19] Use the hashtable to accumulate completion results.

Doug Evans
2015-08-23T17:53:47Z

Keith Seitz <keiths@redhat.com> writes:
> There are no revisions in this version.
>
> --
>
> The completion API now uses a VEC (char_ptr) to collect results of
> completion. The completion tracker is a hashtable whose elements are
> the completions. Both hold essentially the same data, so there is no
> need to keep both around.
>
> This patch introduces some API support for removing the vector of
> completions altogether. While it does not remove the vector or
> the vector return result from the completer functions, this patch
> does not use the results of the vector at all. Only the results of
> the hashtable inside the completer's private data is used.
>
> The vector will be removed in the next patch.
>
> gdb/ChangeLog
>
> * cli/cli-decode.c (complete_on_cmdlist): Use get_completion_count
> to ascertain if there are any completion results.
> * completer.c (remove_leading_fn_component): New function.
> (location_completer): Use get_completion_count to figure out
> how many symbols and/or file names were found searching for
> possible completions.
> Traverse the completion tracker hashtable to strip leading
> file name components. Contents moved to remove_leading_fn_component.
> (free_completer_data): Change argument to proper type.
> (free_entry_callback): New function.
> (free_all_completer_data): New function.
> (vectorize_htab): New function.
> (get_completion_list): New function.
> (get_completion_count): New function.
> (maybe_add_completion): Accumulate completions when not limiting
> the number of completions.
> (complete_line): Ignore the return list from complete_line_internal
> and get the completion results from the tracker.
> Do not count/limit the results at all -- it is no longer necessary.
> Use free_all_completer_data to free any allocated memory during
> completion in the case of an exception.
> Use free_completer_data after get_completion_list to free
> completer data structures.
> (gdb_completion_word_break_characters): Ignore the return list
> from complete_line_internal and get the completion results from
> the tracker.
> Use free_completer_data after get_completion_list to free
> completer data structures.
> * completer.h (get_completion_list): Declare.
> (get_completion_count): Declare.
> * python/py-cmd.c (cmdpy_completer): Use get_completion_count
> to ascertain if there are any completion results.

Hi.
A few nits.

> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index fa95ee6..9691fd1 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -1781,7 +1781,7 @@ complete_on_cmdlist (struct completer_data *cdata,
> commands. If we see no matching commands in the first pass, and
> if we did happen to see a matching deprecated command, we do
> another loop to collect those. */
> - for (pass = 0; matchlist == 0 && pass < 2; ++pass)
> + for (pass = 0; get_completion_count (cdata) == 0 && pass < 2; ++pass)
> {
> for (ptr = list; ptr; ptr = ptr->next)
> if (!strncmp (ptr->name, text, textlen)
> diff --git a/gdb/completer.c b/gdb/completer.c
> index 651e9c2..6faed31 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -197,6 +197,20 @@ filename_completer (struct completer_data *cdata,
> return return_val;
> }
>
> +/* A hashtable traversal function to remove leading file name
> + components, as required by rl_complete. See more detailed explanation
> + in location_completer for more. */
> +
> +static int
> +remove_leading_fn_component (void **slot, void *calldata)
> +{
> + char *fn = *slot;
> + int offset = *(int *) calldata;
> +
> + memmove (fn, fn + offset, strlen (fn) + 1 - offset);
> + return 1;
> +}
> +
> /* Complete on locations, which might be of two possible forms:
>
> file:line
> @@ -286,21 +300,25 @@ location_completer (struct completer_data *cdata,
> {
> list = make_file_symbol_completion_list (cdata, symbol_start, word,
> file_to_match);
> + n_syms = get_completion_count (cdata);
> + n_files = 0;
> xfree (file_to_match);
> }
> else
> {
> list = make_symbol_completion_list (cdata, symbol_start, word);
> + n_syms = get_completion_count (cdata);
> + n_files = 0;
> /* If text includes characters which cannot appear in a file
> name, they cannot be asking for completion on files. */
> if (strcspn (text,
> gdb_completer_file_name_break_characters) == text_len)
> - fn_list = make_source_files_completion_list (cdata, text, text);
> + {
> + fn_list = make_source_files_completion_list (cdata, text, text);
> + n_files = get_completion_count (cdata) - n_syms;
> + }
> }
>
> - n_syms = VEC_length (char_ptr, list);
> - n_files = VEC_length (char_ptr, fn_list);
> -
> /* Catenate fn_list[] onto the end of list[]. */
> if (!n_syms)
> {
> @@ -323,7 +341,7 @@ location_completer (struct completer_data *cdata,
> }
> else if (n_files)
> {
> - char *fn;
> + int offset = word - text;
>
> /* If we only have file names as possible completion, we should
> bring them in sync with what rl_complete expects. The
> @@ -338,13 +356,10 @@ location_completer (struct completer_data *cdata,
> the full "/foo/bar" and "/foo/baz" strings. This produces
> wrong results when, e.g., there's only one possible
> completion, because rl_complete will prepend "/foo/" to each
> - candidate completion. The loop below removes that leading
> + candidate completion. The callback below removes that leading
> part. */
> - for (ix = 0; VEC_iterate (char_ptr, list, ix, fn); ++ix)
> - {
> - memmove (fn, fn + (word - text),
> - strlen (fn) + 1 - (word - text));
> - }
> + htab_traverse (cdata->tracker, remove_leading_fn_component,
> + &offset);

====
I don't know that it'd make a difference, technically,
but use htab_traverse_noresize throughout here just to obviate the need
for the reader to have to think about it?

> }
> else if (!n_syms)
> {
> @@ -837,14 +852,67 @@ new_completer_data (int size)
> /* Free the completion data represented by P. */
>
> static void
> -free_completer_data (void *p)
> +free_completer_data (struct completer_data *cdata)
> {
> - struct completer_data *cdata = p;
> -
> htab_delete (cdata->tracker);
> xfree (cdata);
> }
>
> +/* A hashtable traversal function to free the elements of the table. */
> +
> +static int
> +free_entry_callback (void **slot, void *calldata)
> +{
> + char *element = *slot;
> +
> + xfree (element);
> + return 1;
> +}
> +
> +/* A cleanup function to free all data associated with the completer_data
> + given by P. */
> +
> +static void
> +free_all_completer_data (void *p)
> +{
> + struct completer_data *cdata = p;
> +
> + htab_traverse (cdata->tracker, free_entry_callback, NULL);

====
This isn't needed if one specifies a function for the del_f parameter
to htab_create_alloc, which is a good thing to have in general now that
the hashtable "owns" the strings.

> + free_completer_data (cdata);
> +}
> +
> +/* A hashtable traversal function to turn the hashtable keys
> + into a vector. */
> +
> +static int
> +vectorize_htab (void **slot, void *calldata)
> +{
> + char *element = *slot;
> + VEC (char_ptr) **vector = calldata;
> +
> + VEC_safe_push (char_ptr, *vector, element);
> + return 1;
> +}
> +
> +/* See completer.h. */
> +
> +VEC (char_ptr) *
> +get_completion_list (const struct completer_data *cdata)
> +{
> + VEC (char_ptr) *result = NULL;
> +
> + htab_traverse (cdata->tracker, vectorize_htab, &result);
> + return result;
> +}
> +
> +/* See completer.h. */
> +
> +size_t
> +get_completion_count (const struct completer_data *cdata)
> +{
> + return htab_elements (cdata->tracker);
> +}
> +
> /* Add the completion NAME to the list of generated completions if
> it is not there already.
> If max_completions is negative, nothing is done, not even watching
> @@ -855,12 +923,11 @@ maybe_add_completion (struct completer_data *cdata, char *name)
> {
> void **slot;
>
> - if (max_completions < 0)
> - return MAYBE_ADD_COMPLETION_OK;
> if (max_completions == 0)
> return MAYBE_ADD_COMPLETION_MAX_REACHED;
>
> - if (htab_elements (cdata->tracker) >= max_completions)
> + if (max_completions > 0
> + && htab_elements (cdata->tracker) >= max_completions)
> return MAYBE_ADD_COMPLETION_MAX_REACHED;
>
> slot = htab_find_slot (cdata->tracker, name, INSERT);
> @@ -870,7 +937,8 @@ maybe_add_completion (struct completer_data *cdata, char *name)
>
> *slot = name;
>
> - return (htab_elements (cdata->tracker) < max_completions
> + return ((max_completions < 0
> + || htab_elements (cdata->tracker) < max_completions)
> ? MAYBE_ADD_COMPLETION_OK
> : MAYBE_ADD_COMPLETION_OK_MAX_REACHED);
> }
> @@ -968,66 +1036,20 @@ throw_max_completions_reached_error (void)
> VEC (char_ptr) *
> complete_line (const char *text, const char *line_buffer, int point)
> {
> - VEC (char_ptr) *list;
> - VEC (char_ptr) *result = NULL;
> - struct cleanup *cdata_cleanup, *list_cleanup;
> - char *candidate;
> - int ix;
> + VEC (char_ptr) *result;
> + struct cleanup *cdata_cleanup;
> struct completer_data *cdata;
>
> if (max_completions == 0)
> return NULL;
>
> cdata = new_completer_data (max_completions);
> - cdata_cleanup = make_cleanup (free_completer_data, cdata);
> - list = complete_line_internal (cdata, text, line_buffer, point,
> - handle_completions);
> - if (max_completions < 0)
> - {
> - do_cleanups (cdata_cleanup);
> - return list;
> - }
> -
> - list_cleanup = make_cleanup_free_char_ptr_vec (list);
> -
> - /* If complete_line_internal returned more completions than were
> - recorded by the completion tracker, then the completer function that
> - was run does not support completion tracking. In this case,
> - do a final test for too many completions.
> -
> - Duplicates are also removed here. Otherwise the user is left
> - scratching his/her head: readline and complete_command will remove
> - duplicates, and if removal of duplicates there brings the total under
> - max_completions the user may think gdb quit searching too early. */

====
I'd like to keep this comment in some form.
I think it's important to document why we remove duplicates.

> -
> - if (VEC_length (char_ptr, list) > htab_elements (cdata->tracker))
> - {
> - enum add_completion_status max_reached = ADD_COMPLETION_OK;
> -
> - /* Clear the tracker so that we can re-use it to count the number
> - of returned completions. */
> - htab_empty (cdata->tracker);
> -
> - for (ix = 0; (max_reached == ADD_COMPLETION_OK
> - && VEC_iterate (char_ptr, list, ix, candidate)); ++ix)
> - {
> - max_reached = add_completion (cdata, &result, candidate, NULL, NULL);
> - }
> -
> - /* The return result has been assembled and the original list from
> - complete_line_internal is no longer needed. Free it. */
> - do_cleanups (list_cleanup);
> - }
> - else
> - {
> - /* There is a valid tracker for the completion -- simply return
> - the completed list. */
> - discard_cleanups (list_cleanup);
> - result = list;
> - }
> -
> - do_cleanups (cdata_cleanup);
> -
> + cdata_cleanup = make_cleanup (free_all_completer_data, cdata);
> + complete_line_internal (cdata, text, line_buffer, point,
> + handle_completions);
> + result = get_completion_list (cdata);
> + discard_cleanups (cdata_cleanup);
> + free_completer_data (cdata);
> return result;
> }
>
> @@ -1175,10 +1197,12 @@ gdb_completion_word_break_characters (void)
> struct cleanup *cleanup;
>
> cdata = new_completer_data (max_completions);
> - cleanup = make_cleanup (free_completer_data, cdata);
> - list = complete_line_internal (cdata, rl_line_buffer, rl_line_buffer,
> - rl_point, handle_brkchars);
> - do_cleanups (cleanup);
> + cleanup = make_cleanup (free_all_completer_data, cdata);
> + complete_line_internal (cdata, rl_line_buffer, rl_line_buffer,
> + rl_point, handle_brkchars);
> + list = get_completion_list (cdata);
> + discard_cleanups (cleanup);
> + free_completer_data (cdata);
> gdb_assert (list == NULL);
> return rl_completer_word_break_characters;
> }
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 98b7d14..07c7d93 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -132,6 +132,15 @@ extern const char *skip_quoted (const char *);
>
> extern int get_maximum_completions (void);
>
> +/* Get the list of completions. */
> +
> +extern VEC (char_ptr) *
> + get_completion_list (const struct completer_data *cdata);
> +
> +/* Get the number of completions in CDATA. */
> +
> +extern size_t get_completion_count (const struct completer_data *cdata);
> +
> /* Return values for add_completion. */
>
> enum add_completion_status
> diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
> index 36e352e..cf76bd1 100644
> --- a/gdb/python/py-cmd.c
> +++ b/gdb/python/py-cmd.c
> @@ -406,7 +406,7 @@ cmdpy_completer (struct completer_data *cdata,
>
> /* If we got some results, ignore problems. Otherwise, report
> the problem. */
> - if (result != NULL && PyErr_Occurred ())
> + if (get_completion_count (cdata) > 0 && PyErr_Occurred ())
> PyErr_Clear ();
> }
>