From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19002 invoked by alias); 24 Aug 2015 16:06:49 -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 18991 invoked by uid 89); 24 Aug 2015 16:06:48 -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-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-GCM-SHA256 encrypted) ESMTPS; Mon, 24 Aug 2015 16:06:45 +0000 Received: by pacti10 with SMTP id ti10so27074930pac.0 for ; Mon, 24 Aug 2015 09:06:43 -0700 (PDT) X-Received: by 10.66.147.68 with SMTP id ti4mr46918921pab.90.1440432403594; Mon, 24 Aug 2015 09:06: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 u10sm17907521pbs.16.2015.08.24.09.06.42 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 24 Aug 2015 09:06:43 -0700 (PDT) From: Doug Evans To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 02/19] Remove completion_tracker_t from the public completion API. References: <20150806191404.32159.50755.stgit@valrhona.uglyboxes.com> <20150806191455.32159.34003.stgit@valrhona.uglyboxes.com> Date: Mon, 24 Aug 2015 16:06:00 -0000 In-Reply-To: (Doug Evans's message of "Sat, 22 Aug 2015 18:01:33 -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/msg00683.txt.bz2 Doug Evans writes: > Keith Seitz writes: >>... >> gdb/ChangeLog >> >> PR gdb/17960 >> * completer.c (struct completer_data) : Change type to >> htab_t. >> (enum maybe_add_completion_enum): Make private; moved from >> completer.h. >> [DEFAULT_MAX_COMPLETIONS]: Define. >> (new_completion_tracker): Remove function. >> (new_completer_data): New function. >> (free_completion_tracker): Remove function. >> (free_completer_data): New function. >> (make_cleanup_free_completion_tracker): Remove function. >> (maybe_add_completion): Make static. Update comments. >> Use completer_data instead of completion_tracker_t. >> (completer_strdup): New function. >> (add_completion): New function. >> (complete_line, gdb_completion_word_break_characters): Use >> completer_data instead of completion_tracker. >> * completer.h (completion_tracker_t, new_completion_tracker) >> (make_cleanup_free_completion_tracker, maybe_add_completion) >> (enum maybe_add_completion): Remove declarations. >> (enum add_completion_status): Define. >> (add_completion): Declare. >> * symtab.c (completion_tracker): Remove variable. >> (completion_list_add_name): Use add_completion instead of >> maybe_add_completion and partial copy. >> (default_make_symbol_completion_list_break_on_1): Do not use >> completion_tracker. >> >> gdb/testsuite/ChangeLog >> >> PR gdb/17960 >> * gdb.base/completion.exp: Add some basic tests for the >> location completer, including a regression test for >> the gdb/17960 assertion failure. >>... >> @@ -803,48 +824,41 @@ complete_line_internal (struct completer_data *cdata, >> >> /* See completer.h. */ >> >> -int max_completions = 200; >> +#define DEFAULT_MAX_COMPLETIONS 200 >> +int max_completions = DEFAULT_MAX_COMPLETIONS; >> >> -/* See completer.h. */ >> +/* Allocate a new completer data structure. */ >> >> -completion_tracker_t >> -new_completion_tracker (void) >> +static struct completer_data * >> +new_completer_data (int size) >> { >> - if (max_completions <= 0) >> - return NULL; >> + struct completer_data *cdata = XCNEW (struct completer_data); >> >> - return htab_create_alloc (max_completions, >> - htab_hash_string, (htab_eq) streq, >> - NULL, xcalloc, xfree); >> + cdata->tracker >> + = htab_create_alloc ((size < 0 ? DEFAULT_MAX_COMPLETIONS : size), >> + htab_hash_string, (htab_eq) streq, NULL, > > ==== >... > I'm also slightly ambivalent on creating the hash table if > max_completions is disabled (<= 0), and leaning towards not creating it > to avoid potential confusion. > Let's do the latter (pending reading the rest of the patchset - maybe > there's a good reason to always create it). Coming back to this now that I've gone through the entire series. I now understand why the hashtable is always created. :-) -> because it's now the only repository of completions. So that's ok. As I read the code the question "What does htab_create_alloc do if I pass it a size of zero?" comes to mind. It'll just start with a table size of the first prime it uses. So that's ok. I then checked the callers of new_completer_data: complete_line does this: if (max_completions == 0) return NULL; but gdb_completion_word_break_characters doesn't have this check so new_completer_data has to handle any value for max_completions. I don't think there's a problem here (IIUC gdb_completion_word_break_characters correctly). At the least it would be good to specify in the function comment of new_completer_data that any value for size (-ve, zero, +ve) is ok.