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

[Doug Evans <xdje42 at gmail dot com>]

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 ();
>      }
>