This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 02/18] Remove completion_tracker_t from the public completion API.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]