Re: [PATCH 00/18] Implement full completer limiting

[Doug Evans <dje@google.com>]

Keith Seitz writes:
 > On 04/19/2015 08:21 PM, Doug Evans wrote:
 > > I've gone over the entire patch set.  A few things I like, but there's
 > > at least one thing I'm concerned about.  Replicating the above switch
 > > in each completer: IWBN to avoid such duplication.
 > > We should still be able to remove the global state and fix 17960.
 > 
 > The original patch series (necessarily IMO) exposed this
 > memory-management issue, and a global maintainer approved the change.
 > Surely the approving maintainer realized that this code would propagate
 > to other completer functions eventually, no? [I certainly hope the
 > maintainer did not accept that the complete_line hack would be long-lived!]

Just to make sure we're on the same page,
which complete_line hack?

 > I can certainly change the series to add a function which attempts to
 > hide this detail. For example:
 > 
 > /* Returns whether the maximum number of completions has been
 > reached/exceeded.  */
 > 
 > int
 > add_completion (struct completer_data *cdata, const char *match)
 > {
 >   char *match = xstrdup (name);
 >   enum maybe_add_completion_enum add_status;
 > 
 >   add_status = maybe_add_completion (cdata, match);
 >   switch (add_status)
 >     {
 >     case MAYBE_ADD_COMPLETION_OK:
 >       VEC_safe_push (char_ptr, cdata->results, match);
 >       break;
 >     case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
 >       VEC_safe_push (char_ptr, cdata->results, match);
 >       return 1;
 >     case MAYBE_ADD_COMPLETION_MAX_REACHED:
 >       xfree (match);
 >       return 1;
 >     case MAYBE_ADD_COMPLETION_DUPLICATE:
 >       xfree (match);
 >       break;
 >     }
 > 
 >   return 0;
 > }
 > 
 > Given that completions (like symbol reading/searching) are extremely
 > time-consuming, one of my design goals was to not introduce anything
 > that might slow down completion, including multiple allocations and copying.

Is there any data that says malloc/copying plays a significant role
in the time?  At this point the main culprit is file i/o and(/or) debug
info processing / symbol table processing.

I can imagine a case where the number of completions is large enough
that malloc/copying becomes an issue, but I think the number of
completions would have to be pretty large, far higher than any
reasonable value a user might set for max-completions.
[for some definition of "reasonable"]

So at this point I haven't been worrying about it.
We're already doing a fair bit of malloc/copying in the
code that's there today, e.g., xstrdup calls in complete_line,
and every user I talk to is thrilled with the feature.
[Maybe in a few years when they've forgotten it used to take
minutes will they start to complain about the seconds. :-)]

 > Unfortunately with this goal in mind, the use of this proposed function
 > (alone) can only be pushed into four of the fourteen functions dealing
 > with completions.
 > 
 > The ten remaining functions would require an additional malloc/copy or
 > yet another API function, e.g.,
 > add_completion_1/add_completionA/add_completion_Ex [:-)] to deal with
 > this case. All this to simply forgo checking the return result of
 > maybe_add_completion. I don't see the benefit.
 > 
 > Developers already have to deal with memory management in exception
 > handling, which is more complex than simply checking the return result
 > of a function -- especially in (what I would perceive as) cut-n-paste
 > code like this.

Any time one duplicates code one needs to consider the increased
maintenance burden. Plus less code is easier to read/digest.
I'd like to avoid the duplication where I can.

 > Or perhaps you had another idea in mind?

Some completers don't really need to track the count,
(e.g., even if there were 10000 signals, I doubt it'd matter if
signal_completer tracked them - interesting experiment though),
and the final pass performs its own limiting, so one thing
I have in mind is that individual tracking by every completer
is unnecessary.

Another thought I had was, for individual completers that do need
to track the result, to what extent can they all use the same
API provided by completion tracking (e.g., have an API call
that contains the switch() and have the completers call it, like
you've got above).
That way the quantity of these switches doesn't proliferate.

It may be that this API function can handle all completers,
in which case great, signal_completer et.al. can use it too.
I'm just not big on making it a requirement.
[Though of course I would like the consistency. :-)]

 > In any case, let me know what you'd like me to do, and I'll get right at it.

Does the above make sense?