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

Doug Evans
2014-12-10T16:25:59Z

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.