From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128787 invoked by alias); 31 Jul 2015 00:28:13 -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 128773 invoked by uid 89); 31 Jul 2015 00:28:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 31 Jul 2015 00:28:11 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id F2832A2C03; Fri, 31 Jul 2015 00:28:09 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6V0S8Dj003564 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 30 Jul 2015 20:28:09 -0400 Message-ID: <55BAC118.2000403@redhat.com> Date: Fri, 31 Jul 2015 00:28:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [PATCH v2 02/18] Remove completion_tracker_t from the public completion API. References: <047d7b2e14fb42433305169ffc20@google.com> In-Reply-To: <047d7b2e14fb42433305169ffc20@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00929.txt.bz2 On 05/21/2015 04:34 PM, Doug Evans wrote: > Keith Seitz writes: > [snip] I'm skipping/collapsing several questions/concerns/comments that you've had because I've changed the interface a bit to accommodate some of your later questions/concerns/comments. Hopefully I haven't pruned too much away. :-) > > - /* Do a final test for too many completions. Individual > completers may > > - do some of this, but are not required to. 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. */ > > - > > - for (ix = 0, max_reached = 0; > > - !max_reached && VEC_iterate (char_ptr, list, ix, candidate); > > - ++ix) > > { > > - enum maybe_add_completion_enum add_status; > > + do_cleanups (cdata_cleanup); > > + return list; > > + } > > + > > + list_cleanup = make_cleanup_free_char_ptr_vec (list); > > > > - add_status = maybe_add_completion (tracker, candidate); > > + /* 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. > > > > - switch (add_status) > > + 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. */ > > + > > If we still allow completers to not use the tracker machinery then > there's a u/i issue here: if less than the max is found then dups > aren't removed, whereas if >= max is found then dups are removed. > The difference in behaviour is odd. > Would it hurt to always perform this pass (IOW remove the "if") ? The end point of this series will require completers to use add_completion. I don't like major interfaces like this being "optional," which is why I started this. While this series/patch removes the current concept of a "tracker," it really doesn't. It just hides it all behind the scenes (removes it from the public API). The first several patches simply maintain the status quo, allowing completers to ignore the new API entirely (like almost all completers ignore the `treacker' today). This is kept around only temporarily so that I don't need to convert every completer in one go. Or at least that's what I originally thought, since I actually have converted every completer by the end of this series. [This should keep buildbot happy, too.] However, I don't follow the ui issue you've identified. > 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)) This branch is taken when there is no completion limiting implemented by the completer (htab_elements will probably be zero). In that case, we manually run over the entire VEC returned from the completer, using add_completion to keep track of the list we will eventually return. add_completion itself does duplicate detection/elimination. In the second branch, a fully compliant completion-limiting completer was used. That completer is already using add_completion to register valid completions. Once again, add_completion uses the completer_data's internal htable to track completions and eliminate duplicates. Of course, in the end, when this whole series is committed, all of this code goes away anyhow. No user will ever see it. What have I missed? > OTOH, we *could* *require/expect* all completers to use the tracker > machinery. That is the plan, but they will use it "behind the scenes" via add_completion. > In which case, do we need completers to return a vector of strings? > [ultimately we do, but internally we don't] Right. You're having visions of a later patch in the series. :-) > We could instead just use the hash table as the only repository of the > strings, and then create the vector of strings here from the hashtab. > In the case when completion limiting is turned off we don't want to > use the hashtab to record strings: we can still keep the list in cdata, > as a vector of strings (and leave the hashtab unused). Right. Or we can keep the htab, remove the vec, and still get automatic duplicate elimination. [I cannot think of a scenario where duplicate entires would be useful. This is the final patch in the series.] All this appears (as you now know) after all the completer functions in gdb are converted to using add_completion. > > + NAME should be malloc'd by the caller. That memory is now > > + under control of the completer and should not be freed by the > caller. */ > > Note to self: What's the cost of removing the "caller must xstrdup" > requirement? Not much. It's just another function. The reason I kept it this way is because some (but not all) completers manipulate the result based on text/word. [more on that below] > I see a pattern in several(all?) completers. This is the "manipulation" I referred to above. I've modified add_completion to take (option) text and word parameters. These are passed to a new completer_strdup function which does all this. This eliminates the need to malloc the argument to add_completion. > If we're going to do a fullscale completion API cleanup we should > at least think about how we'd address this (IOW simplify it and remove > all the duplication: it's excessive and warrants attention). > We don't have to implement it in this pass, just understand what we > want to do (if anything). > Seems like all of this logic could be collapsed into a function > that then calls add_completion or some such. I did something similar. Basically: enum add_completion_status add_completion (struct completer_data *cdata, const char *match, const char *text, const char *word) { char *alloc = completer_strdup (match, text, word); /* rest of add_completion pretty much unchanged */ } And completer_strdup will do the common copying seen all over the place. If either `text' or `word' is NULL, then it strdups the completion. Does that sound like a plan? Keith