Re: [PATCH] decorate: clean up and document API

On Fri, 8 Dec 2017 04:55:11 -0500
Jeff King [off-list ref] wrote:

I have mixed feelings. On the one hand, compiling and running the code
ensures that those things actually work. On the other hand, I expect you
can make a much clearer example if instead of having running code, you
show snippets of almost-code.

E.g.:

  struct decoration d = { NULL };

  add_decoration(&d, obj, "foo");
  ...
  str = lookup_decoration(obj);

pretty much gives the relevant overview, with very little boilerplate.
Yes, it omits things like the return value of add_decoration(), but
those sorts of details are probably better left to the function
docstrings.

The part about iterating over all entries should probably also be shown
too. Besides that, I'm OK with having a simplified example in
documentation too, but I'll wait and see if others have any opinions
before making any changes.

Other than that philosophical point, the documentation you added looks
pretty good to me. Two possible improvements to the API we could do on
top:

  1. Should there be a DECORATION_INIT macro (possibly taking the "name"
     as an argument)? (Actually, the whole name thing seems like a
     confusing and bad API design in the first place).

Agreed about the "name" thing. I'll add a DECORATION_INIT when I make
the next reroll, but I think that having it with no argument is best
(and instantiating "name" with NULL).

  2. This is really just an oidmap to a void pointer. I wonder if we
     ought to be wrapping that code (I think we still want some
     interface so that the caller doesn't have to declare their own
     structs).

It is slightly different from oidmap in that this uses "struct object *"
as a key whereas oidmap uses "struct object_id", meaning that a user of
"decorate" must already have objects allocated or be willing to allocate
them, whereas a user of "oidmap" doesn't.

Having said that, it is true that perhaps we have too many data
structures doing similar things.