From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24902 invoked by alias); 5 Dec 2014 23:54:16 -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 24890 invoked by uid 89); 5 Dec 2014 23:54:15 -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-f47.google.com Received: from mail-pa0-f47.google.com (HELO mail-pa0-f47.google.com) (209.85.220.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 05 Dec 2014 23:54:14 +0000 Received: by mail-pa0-f47.google.com with SMTP id kq14so1623645pab.20 for ; Fri, 05 Dec 2014 15:54:12 -0800 (PST) X-Received: by 10.70.135.202 with SMTP id pu10mr216084pdb.64.1417823652227; Fri, 05 Dec 2014 15:54:12 -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 oa8sm30054721pdb.84.2014.12.05.15.54.10 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 05 Dec 2014 15:54:11 -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> Date: Fri, 05 Dec 2014 23:54:00 -0000 In-Reply-To: <1417094168-25868-4-git-send-email-gbenson@redhat.com> (Gary Benson's message of "Thu, 27 Nov 2014 13:16:08 +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/msg00159.txt.bz2 Gary Benson 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) > : 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 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 :-)).