From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14642 invoked by alias); 6 Jul 2008 03:57:10 -0000 Received: (qmail 14633 invoked by uid 22791); 6 Jul 2008 03:57:08 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 06 Jul 2008 03:56:51 +0000 Received: (qmail 9791 invoked from network); 6 Jul 2008 03:56:49 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 6 Jul 2008 03:56:49 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFC] fix annoying completion bug on directories Date: Sun, 06 Jul 2008 03:57:00 -0000 User-Agent: KMail/1.9.9 Cc: "Pierre Muller" References: <000901c8def8$137b1bf0$3a7153d0$@u-strasbg.fr> In-Reply-To: <000901c8def8$137b1bf0$3a7153d0$@u-strasbg.fr> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200807060456.49298.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2008-07/txt/msg00073.txt.bz2 A Sunday 06 July 2008 00:37:22, Pierre Muller wrote: > I was always annoyed by a small but annoying bug in the > gdb TAB completion for directories. > > If you start gdb without args and type: > "(gdb) file /usr/inclu" > and you now type TAB to get the completion, > you would expect to get > "(gdb) file /usr/include/" > but instead you will get > "(gdb) file /usr/include " > but if you delete the "de " and type TAB char > again, you will get the correct answer! > Oh boy, thanks for looking into this! This has been annoying me a lot as well. I went ahead and tried to understand your changes, but beware that my readline+GDB foo is low. > This all looked like some wrongly set parameter... > > It took me a while to figure out what is > going on and why this bug appears: > ... > This explains why at the second try, > the word found by _rl_find_completion_word > is now "/usr/inclu". > Urgl. This means that finding the completion word is always somewhat broken on the next command, when the next command should use a different word break character list? > As the code is rather convoluted, I tried to just > created a "special" case for which only this variable > would be set, without really trying to find > the completions. Did you consider breaking complete_line in two? There's more you could skip that just the completion calls. Hmm, doesn't look like it would be a maintainable result. Basically the parsing, command lookups and most of the comments would have to be duplicated, or a weird abstraction would have to be employed. Not sure if the result would be better at all. > > Luckily, there is a hook function inside > _rl_find_completion_word that allows to > change the default value of the list of chars > considered as marking word start/end. > Yay! > I found that this works nicely... > > I ran the testsuite on cygwin, and found one > change, but in gdb.gdb/selftest. > FAIL (timeout) xgdb is at prompt > > I suspect that this is due to a problem with the > [New thread %s] output, that are now default. > Because this output appears right after the gdb prompt, > and thus the match with a gdb prompt exactly at the end > is not found. In these cases, in addition to analising the log file, please rerun the test to confirm it was spurious. Cygwin testing is indeed flackier than linux. Just do make check RUNTESTFLAGS="failingtest.exp" a couple of times. If it's always failing now, it's a regression. If the failure goes aways, then also retest with with the patch unapplied. You should see the flackiness too. If not, it may be a regression, but you'll have to judge from the logs -- it may just be difficult to reproduce flackiness. > I found no change in the tests dealing with completion, > but I also don't really know how to add such a test... > Take a look at the completion.exp test for the test that does: send_gdb "file ./gdb.base/complet\t" Maybe you could do something similar? Event this fails to complete the '/' the first time without your patch: send_gdb "dir ..\t" > > ChangeLog entry: > > 2008-06-06 Pierre Muller ^ You have to do something about that calendar :-) > > * gdb/completer.c: Include gdb_assert.h Please also update Makefile.in. Still needed until we have automatic dependency tracking... > /* When completing on command names, we remove '-' from the list of > word break characters, since we use it in command names. If the > readline library sees one in any of the current completion strings, > @@ -472,20 +475,29 @@ command_completer (char *text, char *wor > char ** > complete_line (const char *text, char *line_buffer, int point) Please add somewhere describing the change you're making, and why it's needed. > { > + int only_brkchars = 0; > char **list = NULL; > char *tmp_command, *p; > + > /* Pointer within tmp_command which corresponds to text. */ > char *word; > struct cmd_list_element *c, *result_list; > > + /* If text is NULL, we only want to set the variable > + current_gdb_completer_word_break_characters. */ > + if (!text) > + { > + only_brkchars = 1; > + text = line_buffer; > + } > + Another alternative would be to add a new parameter to the function, rename it, and add a new complete_line function as wrapper to the old one: char ** complete_or_set_brkchars (const char *text, char *line_buffer, int point, int what_to_do) { ... } char ** complete_line (const char *text, char *line_buffer, int point) { complete_or_set_brkchars (text, line_buffer, complete_please); } static char * gdb_completion_word_break (void) { complete_or_set_brkchars (rl_line_buffer, rl_line_buffer, rl_point, set_brkchars_please); } > - rl_completer_word_break_characters = > + current_gdb_completer_word_break_characters = > gdb_completer_file_name_break_characters; > } > - > + rl_completer_word_break_characters = > + current_gdb_completer_word_break_characters; > return list; You changed a bunch of rl_completer_word_break_characters to current_gdb_completer_word_break_characters, only to in the end set them to be the same. If you do it the other way around, you'd only be adding a single: current_gdb_completer_word_break_characters = rl_completer_word_break_characters; at the end of complete_line. > } > > +static char * > +gdb_completion_word_break () ^ (void) > +{ > + gdb_assert (complete_line (NULL, rl_line_buffer, rl_point) == NULL); Please, no side effects in assert statements. We always build with them on, but it's bad practice to rely on that. If you want to keep the assert, do it like: char **list = complete_line (NULL, rl_line_buffer, rl_point); gdb_assert (line == NULL); And please add a comment somewhere describing what this is for. > + > + return current_gdb_completer_word_break_characters; > +} from readline.c:complete.c: char _rl_find_completion_word (fp, dp) int *fp, *dp; { int scan, end, found_quote, delimiter, pass_next, isbrk; char quote_char, *brkchars; end = rl_point; found_quote = delimiter = 0; quote_char = '\0'; brkchars = 0; if (rl_completion_word_break_hook) brkchars = (*rl_completion_word_break_hook) (); if (brkchars == 0) brkchars = rl_completer_word_break_characters; It looks like you could get rid of current_gdb_completer_word_break_characters entirely, and return either NULL or rl_completer_word_break_characters in your new hook; or if keeping current_gdb_comp... not set rl_completer_word_break_characters in complete_line at all? Also, doesn't filename_completer always get text == word now? If so, there's some code in there that turns dead (both the if text < word, and the else clauses). > +void > +_initialize_completer (void) > +{ > + rl_completion_word_break_hook = gdb_completion_word_break; Did you consider putting this in init_main near the other rl_ initializations? OTHO, rl_completer_word_break_characters was indeed already mostly only tweaked in this file, and it goes in hand with this hook... -- Pedro Alves