From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10270 invoked by alias); 23 Aug 2015 17:53:47 -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 10256 invoked by uid 89); 23 Aug 2015 17:53:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 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-f52.google.com Received: from mail-pa0-f52.google.com (HELO mail-pa0-f52.google.com) (209.85.220.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 23 Aug 2015 17:53:44 +0000 Received: by pacgr6 with SMTP id gr6so723697pac.1 for ; Sun, 23 Aug 2015 10:53:43 -0700 (PDT) X-Received: by 10.66.160.1 with SMTP id xg1mr37551695pab.27.1440352423154; Sun, 23 Aug 2015 10:53:43 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by smtp.gmail.com with ESMTPSA id ev2sm14558353pbb.37.2015.08.23.10.53.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 23 Aug 2015 10:53:42 -0700 (PDT) From: Doug Evans To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 18/19] Use the hashtable to accumulate completion results. References: <20150806191404.32159.50755.stgit@valrhona.uglyboxes.com> <20150806192131.32159.65241.stgit@valrhona.uglyboxes.com> Date: Sun, 23 Aug 2015 17:53:00 -0000 In-Reply-To: <20150806192131.32159.65241.stgit@valrhona.uglyboxes.com> (Keith Seitz's message of "Thu, 06 Aug 2015 12:21:47 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00659.txt.bz2 Keith Seitz writes: > There are no revisions in this version. > > -- > > The completion API now uses a VEC (char_ptr) to collect results of > completion. The completion tracker is a hashtable whose elements are > the completions. Both hold essentially the same data, so there is no > need to keep both around. > > This patch introduces some API support for removing the vector of > completions altogether. While it does not remove the vector or > the vector return result from the completer functions, this patch > does not use the results of the vector at all. Only the results of > the hashtable inside the completer's private data is used. > > The vector will be removed in the next patch. > > gdb/ChangeLog > > * cli/cli-decode.c (complete_on_cmdlist): Use get_completion_count > to ascertain if there are any completion results. > * completer.c (remove_leading_fn_component): New function. > (location_completer): Use get_completion_count to figure out > how many symbols and/or file names were found searching for > possible completions. > Traverse the completion tracker hashtable to strip leading > file name components. Contents moved to remove_leading_fn_component. > (free_completer_data): Change argument to proper type. > (free_entry_callback): New function. > (free_all_completer_data): New function. > (vectorize_htab): New function. > (get_completion_list): New function. > (get_completion_count): New function. > (maybe_add_completion): Accumulate completions when not limiting > the number of completions. > (complete_line): Ignore the return list from complete_line_internal > and get the completion results from the tracker. > Do not count/limit the results at all -- it is no longer necessary. > Use free_all_completer_data to free any allocated memory during > completion in the case of an exception. > Use free_completer_data after get_completion_list to free > completer data structures. > (gdb_completion_word_break_characters): Ignore the return list > from complete_line_internal and get the completion results from > the tracker. > Use free_completer_data after get_completion_list to free > completer data structures. > * completer.h (get_completion_list): Declare. > (get_completion_count): Declare. > * python/py-cmd.c (cmdpy_completer): Use get_completion_count > to ascertain if there are any completion results. Hi. A few nits. > diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c > index fa95ee6..9691fd1 100644 > --- a/gdb/cli/cli-decode.c > +++ b/gdb/cli/cli-decode.c > @@ -1781,7 +1781,7 @@ complete_on_cmdlist (struct completer_data *cdata, > commands. If we see no matching commands in the first pass, and > if we did happen to see a matching deprecated command, we do > another loop to collect those. */ > - for (pass = 0; matchlist == 0 && pass < 2; ++pass) > + for (pass = 0; get_completion_count (cdata) == 0 && pass < 2; ++pass) > { > for (ptr = list; ptr; ptr = ptr->next) > if (!strncmp (ptr->name, text, textlen) > diff --git a/gdb/completer.c b/gdb/completer.c > index 651e9c2..6faed31 100644 > --- a/gdb/completer.c > +++ b/gdb/completer.c > @@ -197,6 +197,20 @@ filename_completer (struct completer_data *cdata, > return return_val; > } > > +/* A hashtable traversal function to remove leading file name > + components, as required by rl_complete. See more detailed explanation > + in location_completer for more. */ > + > +static int > +remove_leading_fn_component (void **slot, void *calldata) > +{ > + char *fn = *slot; > + int offset = *(int *) calldata; > + > + memmove (fn, fn + offset, strlen (fn) + 1 - offset); > + return 1; > +} > + > /* Complete on locations, which might be of two possible forms: > > file:line > @@ -286,21 +300,25 @@ location_completer (struct completer_data *cdata, > { > list = make_file_symbol_completion_list (cdata, symbol_start, word, > file_to_match); > + n_syms = get_completion_count (cdata); > + n_files = 0; > xfree (file_to_match); > } > else > { > list = make_symbol_completion_list (cdata, symbol_start, word); > + n_syms = get_completion_count (cdata); > + n_files = 0; > /* If text includes characters which cannot appear in a file > name, they cannot be asking for completion on files. */ > if (strcspn (text, > gdb_completer_file_name_break_characters) == text_len) > - fn_list = make_source_files_completion_list (cdata, text, text); > + { > + fn_list = make_source_files_completion_list (cdata, text, text); > + n_files = get_completion_count (cdata) - n_syms; > + } > } > > - n_syms = VEC_length (char_ptr, list); > - n_files = VEC_length (char_ptr, fn_list); > - > /* Catenate fn_list[] onto the end of list[]. */ > if (!n_syms) > { > @@ -323,7 +341,7 @@ location_completer (struct completer_data *cdata, > } > else if (n_files) > { > - char *fn; > + int offset = word - text; > > /* If we only have file names as possible completion, we should > bring them in sync with what rl_complete expects. The > @@ -338,13 +356,10 @@ location_completer (struct completer_data *cdata, > the full "/foo/bar" and "/foo/baz" strings. This produces > wrong results when, e.g., there's only one possible > completion, because rl_complete will prepend "/foo/" to each > - candidate completion. The loop below removes that leading > + candidate completion. The callback below removes that leading > part. */ > - for (ix = 0; VEC_iterate (char_ptr, list, ix, fn); ++ix) > - { > - memmove (fn, fn + (word - text), > - strlen (fn) + 1 - (word - text)); > - } > + htab_traverse (cdata->tracker, remove_leading_fn_component, > + &offset); ==== I don't know that it'd make a difference, technically, but use htab_traverse_noresize throughout here just to obviate the need for the reader to have to think about it? > } > else if (!n_syms) > { > @@ -837,14 +852,67 @@ new_completer_data (int size) > /* Free the completion data represented by P. */ > > static void > -free_completer_data (void *p) > +free_completer_data (struct completer_data *cdata) > { > - struct completer_data *cdata = p; > - > htab_delete (cdata->tracker); > xfree (cdata); > } > > +/* A hashtable traversal function to free the elements of the table. */ > + > +static int > +free_entry_callback (void **slot, void *calldata) > +{ > + char *element = *slot; > + > + xfree (element); > + return 1; > +} > + > +/* A cleanup function to free all data associated with the completer_data > + given by P. */ > + > +static void > +free_all_completer_data (void *p) > +{ > + struct completer_data *cdata = p; > + > + htab_traverse (cdata->tracker, free_entry_callback, NULL); ==== This isn't needed if one specifies a function for the del_f parameter to htab_create_alloc, which is a good thing to have in general now that the hashtable "owns" the strings. > + free_completer_data (cdata); > +} > + > +/* A hashtable traversal function to turn the hashtable keys > + into a vector. */ > + > +static int > +vectorize_htab (void **slot, void *calldata) > +{ > + char *element = *slot; > + VEC (char_ptr) **vector = calldata; > + > + VEC_safe_push (char_ptr, *vector, element); > + return 1; > +} > + > +/* See completer.h. */ > + > +VEC (char_ptr) * > +get_completion_list (const struct completer_data *cdata) > +{ > + VEC (char_ptr) *result = NULL; > + > + htab_traverse (cdata->tracker, vectorize_htab, &result); > + return result; > +} > + > +/* See completer.h. */ > + > +size_t > +get_completion_count (const struct completer_data *cdata) > +{ > + return htab_elements (cdata->tracker); > +} > + > /* Add the completion NAME to the list of generated completions if > it is not there already. > If max_completions is negative, nothing is done, not even watching > @@ -855,12 +923,11 @@ maybe_add_completion (struct completer_data *cdata, char *name) > { > void **slot; > > - if (max_completions < 0) > - return MAYBE_ADD_COMPLETION_OK; > if (max_completions == 0) > return MAYBE_ADD_COMPLETION_MAX_REACHED; > > - if (htab_elements (cdata->tracker) >= max_completions) > + if (max_completions > 0 > + && htab_elements (cdata->tracker) >= max_completions) > return MAYBE_ADD_COMPLETION_MAX_REACHED; > > slot = htab_find_slot (cdata->tracker, name, INSERT); > @@ -870,7 +937,8 @@ maybe_add_completion (struct completer_data *cdata, char *name) > > *slot = name; > > - return (htab_elements (cdata->tracker) < max_completions > + return ((max_completions < 0 > + || htab_elements (cdata->tracker) < max_completions) > ? MAYBE_ADD_COMPLETION_OK > : MAYBE_ADD_COMPLETION_OK_MAX_REACHED); > } > @@ -968,66 +1036,20 @@ throw_max_completions_reached_error (void) > VEC (char_ptr) * > complete_line (const char *text, const char *line_buffer, int point) > { > - VEC (char_ptr) *list; > - VEC (char_ptr) *result = NULL; > - struct cleanup *cdata_cleanup, *list_cleanup; > - char *candidate; > - int ix; > + VEC (char_ptr) *result; > + struct cleanup *cdata_cleanup; > struct completer_data *cdata; > > if (max_completions == 0) > return NULL; > > cdata = new_completer_data (max_completions); > - cdata_cleanup = make_cleanup (free_completer_data, cdata); > - list = complete_line_internal (cdata, text, line_buffer, point, > - handle_completions); > - if (max_completions < 0) > - { > - do_cleanups (cdata_cleanup); > - return list; > - } > - > - list_cleanup = make_cleanup_free_char_ptr_vec (list); > - > - /* If complete_line_internal returned more completions than were > - recorded by the completion tracker, then the completer function that > - was run does not support completion tracking. In this case, > - do a final test for too many completions. > - > - Duplicates are also removed here. Otherwise the user is left > - scratching his/her head: readline and complete_command will remove > - duplicates, and if removal of duplicates there brings the total under > - max_completions the user may think gdb quit searching too early. */ ==== I'd like to keep this comment in some form. I think it's important to document why we remove duplicates. > - > - if (VEC_length (char_ptr, list) > htab_elements (cdata->tracker)) > - { > - enum add_completion_status max_reached = ADD_COMPLETION_OK; > - > - /* Clear the tracker so that we can re-use it to count the number > - of returned completions. */ > - htab_empty (cdata->tracker); > - > - for (ix = 0; (max_reached == ADD_COMPLETION_OK > - && VEC_iterate (char_ptr, list, ix, candidate)); ++ix) > - { > - max_reached = add_completion (cdata, &result, candidate, NULL, NULL); > - } > - > - /* The return result has been assembled and the original list from > - complete_line_internal is no longer needed. Free it. */ > - do_cleanups (list_cleanup); > - } > - else > - { > - /* There is a valid tracker for the completion -- simply return > - the completed list. */ > - discard_cleanups (list_cleanup); > - result = list; > - } > - > - do_cleanups (cdata_cleanup); > - > + cdata_cleanup = make_cleanup (free_all_completer_data, cdata); > + complete_line_internal (cdata, text, line_buffer, point, > + handle_completions); > + result = get_completion_list (cdata); > + discard_cleanups (cdata_cleanup); > + free_completer_data (cdata); > return result; > } > > @@ -1175,10 +1197,12 @@ gdb_completion_word_break_characters (void) > struct cleanup *cleanup; > > cdata = new_completer_data (max_completions); > - cleanup = make_cleanup (free_completer_data, cdata); > - list = complete_line_internal (cdata, rl_line_buffer, rl_line_buffer, > - rl_point, handle_brkchars); > - do_cleanups (cleanup); > + cleanup = make_cleanup (free_all_completer_data, cdata); > + complete_line_internal (cdata, rl_line_buffer, rl_line_buffer, > + rl_point, handle_brkchars); > + list = get_completion_list (cdata); > + discard_cleanups (cleanup); > + free_completer_data (cdata); > gdb_assert (list == NULL); > return rl_completer_word_break_characters; > } > diff --git a/gdb/completer.h b/gdb/completer.h > index 98b7d14..07c7d93 100644 > --- a/gdb/completer.h > +++ b/gdb/completer.h > @@ -132,6 +132,15 @@ extern const char *skip_quoted (const char *); > > extern int get_maximum_completions (void); > > +/* Get the list of completions. */ > + > +extern VEC (char_ptr) * > + get_completion_list (const struct completer_data *cdata); > + > +/* Get the number of completions in CDATA. */ > + > +extern size_t get_completion_count (const struct completer_data *cdata); > + > /* Return values for add_completion. */ > > enum add_completion_status > diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c > index 36e352e..cf76bd1 100644 > --- a/gdb/python/py-cmd.c > +++ b/gdb/python/py-cmd.c > @@ -406,7 +406,7 @@ cmdpy_completer (struct completer_data *cdata, > > /* If we got some results, ignore problems. Otherwise, report > the problem. */ > - if (result != NULL && PyErr_Occurred ()) > + if (get_completion_count (cdata) > 0 && PyErr_Occurred ()) > PyErr_Clear (); > } >