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

[Gary Benson <gbenson@redhat.com>]

Doug Evans wrote:
> 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.
> 
> 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?]

It's a nice idea but I'm not volunteering to implement it :)
I already spent too much time figuring out how to thread things
through readline.

> 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.

rl_completion_query_items is a different thing.
From http://web.mit.edu/gnu/doc/html/rlman_2.html:

  Up to this many items will be displayed in response to a possible-
  completions call. After that, we ask the user if she is sure she
  wants to see them all. The default value is 100.

Basically it's the threshold readline uses to decide whether to say
"Display all 159 possibilities? (y or n)" or just print all the
candidates without asking.
  
> 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?

GDB's "remotetimeout" and "trace-buffer-size" options use -1 to denote
unlimited, as do Scheme things defined with PARAM_ZUINTEGER_UNLIMITED.
I prefer "max-completions" being consistent with other GDB options
over "max-completions" being consistent with readline options.
It's not important to me that users can disable completion, I put the
functionality there because existing GDB code uses -1 so I had to
handle 0 somehow.

> 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.

I think we want both.

> At the least, it'd probably be good to mention how both interact in
> in the docs.

I can do this.

> > 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 matche> > [...]
> > +	  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 ();
> > +	    }
> 
> 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 :-)).

I'm no TUI expert, so I can't comment on whether it's necessary or
not.  Assuming it is necessary, I don't know how I could remove this
block without vectorizing the CLI/TUI interface... or is this done
already?  There are other places where "#ifdef TUI" and variants
are used, at least four:

  gdb/cli/cli-cmds.c
  gdb/main.c
  gdb/printcmd.c
  gdb/utils.c

Cheers,
Gary

-- 
http://gbenson.net/