Re: [PATCH 3/3 v2] Implement completion limiting

[Doug Evans <xdje42 at gmail dot com>]

Gary Benson <gbenson@redhat.com> writes:

> This commit adds a new exception, TOO_MANY_COMPLETIONS_ERROR, to be
> thrown whenever the completer has generated too many candidates to
> be useful.  A new user-settable variable, "max_completions", is added
> to control this behaviour.  A top-level completion limit is added to
> complete_line_internal, as the final check to ensure the user never
> sees too many completions.  An additional limit is added to
> default_make_symbol_completion_list_break_on, to halt time-consuming
> symbol table expansions.
>
> gdb/ChangeLog:
>
> 	PR cli/9007
> 	PR cli/11920
> 	PR cli/15548
> 	* common/common-exceptions.h (enum errors)
> 	<TOO_MANY_COMPLETIONS_ERROR>: New value.
> 	* completer.h (completion_tracker_t): New typedef.
> 	(new_completion_tracker): New declaration.
> 	(make_cleanup_free_completion_tracker): Likewise.
> 	(maybe_limit_completions): Likewise.
> 	* completer.c [TUI]: Include tui/tui.h and tui/tui-io.h.
> 	(max_completions): New static variable.
> 	(new_completion_tracker): New function.
> 	(make_cleanup_free_completion_tracker): Likewise.
> 	(maybe_limit_completions): Likewise.
> 	(complete_line_internal): Do not generate any completions if
> 	max_completions = 0.  Limit the number of completions if
> 	max_completions >= 0.
> 	(line_completion_function): Handle TOO_MANY_COMPLETIONS_ERROR.
> 	(_initialize_completer): New declaration and function.
> 	* symtab.c: Include completer.h.
> 	(completion_tracker): New static variable.
> 	(completion_list_add_name): Call maybe_limit_completions.
> 	(default_make_symbol_completion_list_break_on): Maintain
> 	completion_tracker across calls to completion_list_add_name.
> 	* NEWS (New Options): Mention set/show max-completions.
>
> gdb/doc/ChangeLog:
>
> 	* gdb.texinfo (Command Completion): Document new
> 	"set/show max-completions" option.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/completion.exp: Disable completion limiting for
> 	existing tests.  Add new tests to check completion limiting.
> 	* gdb.linespec/ls-errs.exp: Disable completion limiting.

Hi.

I played with the patch a bit.
I have a few questions on the u/i.

1) IWBN if, when "Too many possibilities" is hit, the user was still
shown the completions thus far.  I'd rather not have to abort
the command I'm trying to do, increase max-completions, and
then try again (or anything else to try to find what I'm looking
for in order to complete the command).  At least not if I don't have to:
the completions thus far may provide a hint at what I'm looking for.
Plus GDB has already computed them, might as well print them.
Imagine if the total count is MAX+1, the user might find it annoying
to not be shown anything just because the count is one beyond the max.

So instead of "Too many possibilities", how about printing
the completions thus far and then include a message saying
the list is clipped due to max-completions being reached?
[Maybe readline makes this difficult, but I think
it'd be really nice have. Thoughts?]

2) Readline has a limiting facility already: rl_completion_query_items.
But it's only applied after all completions have been computed so
it doesn't help us.
It would be good for the docs to explain the difference.
E.g. with the default of 200 for max-completions, if I do

(top-gdb) b dwarf2<tab><tab>

I get

Display all 159 possibilities? (y or n)

As a user, I'm kinda wondering why I'm being asked this if, for example,
I've explicitly set max-completions to some value.
[I know normally users might not even be aware of max-completions,
but suppose for the sake of discussion that they've set it to some value
larger than rl_completion_query_items.]
Note: rl_completion_query_items can be set in ~/.inputrc with
the completion-query-items parameter.

3) rl_completion_query_items uses a value of zero to mean unlimited,
whereas max_completions uses -1 (or "unlimited").
While it might be nice to provide a way to disable completions
completely (by setting max-completions to zero), I'm trying to decide
whether that benefit is sufficient to justify the inconsistency with
rl_completion_query_items.  Thoughts?

4) Is there a use-case that involves wanting both
rl_completion_query_items and max_completions parameters?
Certainly we need to limit the number while we're building them,
but do we need both parameters? [IOW, could we fold them into one?]
I can imagine wanting to set max-completions to some large value
but still be given a prompt if the completion-query-items
threshold is reached, so I think we want both.
I'm just raising the possibility that maybe we don't want both
in case someone wants to comment.
At the least, it'd probably be good to mention how both interact in the docs.

> diff --git a/gdb/completer.c b/gdb/completer.c
> index a0f3fa3..4a2302c 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> [...]
> @@ -894,7 +984,35 @@ line_completion_function (const char *text, int matches,
>  	  VEC_free (char_ptr, list);
>  	}
>        index = 0;
> -      list = complete_line (text, line_buffer, point);
> +
> +      TRY_CATCH (ex, RETURN_MASK_ALL)
> +	list = complete_line (text, line_buffer, point);
> +
> +      if (ex.reason < 0)
> +	{
> +	  if (ex.error != TOO_MANY_COMPLETIONS_ERROR)
> +	    throw_exception (ex);
> +
> +	  if (rl_completion_type != TAB)
> +	    {
> +#if defined(TUI)
> +	      if (tui_active)
> +		{
> +		  tui_puts ("\n");
> +		  tui_puts (ex.message);
> +		  tui_puts ("\n");
> +		}
> +	      else
> +#endif
> +		{
> +		  rl_crlf ();
> +		  fputs (ex.message, rl_outstream);
> +		  rl_crlf ();
> +		}
> +
> +	      rl_on_new_line ();
> +	    }
> +	}
>      }
>  
>    /* If we found a list of potential completions during initialization

Bubbling up TUI implementation details into GDB core gives me pause.
I'm left wondering if there are more problems, and this is just fixing
one of them.  I see that TUI has special code for readline,
(grep for readline in tui-io.c)
so at the least I'm wondering why this is necessary.
And if it is, let's push it down into tui/ as much as possible
(with a comment explaining why the code exists :-)).