From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18588 invoked by alias); 10 Dec 2014 16:25:59 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18576 invoked by uid 89); 10 Dec 2014 16:25:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f54.google.com Received: from mail-pa0-f54.google.com (HELO mail-pa0-f54.google.com) (209.85.220.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 10 Dec 2014 16:25:56 +0000 Received: by mail-pa0-f54.google.com with SMTP id fb1so3084972pad.41 for ; Wed, 10 Dec 2014 08:25:54 -0800 (PST) X-Received: by 10.70.89.207 with SMTP id bq15mr8638656pdb.68.1418228754666; Wed, 10 Dec 2014 08:25:54 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id ml8sm4643387pdb.67.2014.12.10.08.25.52 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 10 Dec 2014 08:25:53 -0800 (PST) From: Doug Evans To: Gary Benson Cc: gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: [PATCH 3/3 v2] Implement completion limiting References: <1417094168-25868-1-git-send-email-gbenson@redhat.com> <1417094168-25868-4-git-send-email-gbenson@redhat.com> <20141210122233.GA7299@blade.nx> Date: Wed, 10 Dec 2014 16:25:00 -0000 In-Reply-To: <20141210122233.GA7299@blade.nx> (Gary Benson's message of "Wed, 10 Dec 2014 12:22:33 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-12/txt/msg00207.txt.bz2 Gary Benson 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 >> >> 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.