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

[Keith Seitz <keiths@redhat.com>]

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!]

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.

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.

Or perhaps you had another idea in mind?

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

Keith