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

[Doug Evans <xdje42 at gmail dot com>]

Gary Benson <gbenson@redhat.com> writes:
> Doug Evans wrote:
>> 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.

One thought I had was one could add a final completion entry
that was the message.
Would that work?

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

I realize that.
Still, the two interact, and we should think through the u/i.

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

OTOH, 

[dje@seba gdb]$ grep add_setshow_uinteger *.c | wc
     11      50     797
[dje@seba gdb]$ grep add_setshow_zuinteger_unlimited *.c | wc
      2       8     163

gdb uses a mix, so consistency with gdb is a bit of a toss up.
>From a u/i perspective does a value of zero for max-completions
make sense?  Maybe it does, but I dunno, it doesn't feel like it.

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

"works for me"

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

Thanks.  btw, I noticed this in completer.c:

/* Generate completions all at once.  Returns a vector of strings
   allocated with xmalloc.  Returns NULL if there are no completions
   or if max_completions is 0.  Throws TOO_MANY_COMPLETIONS_ERROR if
   max_completions is greater than zero and the number of completions
   is greater than max_completions.

But that's not what the code does AFAICT:

  /* Possibly throw TOO_MANY_COMPLETIONS_ERROR.  Individual
     completers may do this too, to avoid unnecessary work,
     but this is the ultimate check that stops users seeing
     more completions than they wanted.  */
  if (max_completions >= 0)

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

If we don't know whether it's necessary then why are we adding it?
["it" being the test for tui_active and the following code]
I don't understand.  Was this derived from another place in gdb
that needed to do a similar thing?  I grepped all uses of tui_active
outside of tui/*.c and didn't see anything.

One hope I had was that this would be enough:

>> > +		  rl_crlf ();
>> > +		  fputs (ex.message, rl_outstream);
>> > +		  rl_crlf ();

and that the efforts tui/*.c goes to to support readline would
make that work regardless of the value of tui_active.
But I confess I haven't tried it.

I wouldn't suggest vectorizing the tui interface.
But I do, at the least, want to understand why this is necessary
("this" being the test for tui_active and the different code
depending on whether it is true or not),
and if it is then I would at a minimum put this code:

>> > +#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 ();

into a function and call it from line_completion_function.
completer.c up until now has had zero references to tui_*,
and adding one with no explanation of why makes the code
hard to reason about.