mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Jonathan Tan <>
Subject: Re: [PATCH] decorate: clean up and document API
Date: Fri, 15 Dec 2017 05:03:50 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Dec 11, 2017 at 10:32:49AM -0800, Jonathan Tan wrote:

> > 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).

That will leave callers like the one in log-tree unable to use the
macro, since it uses a static initializer. I didn't dig, though. That
may be the only one. Most of the rest seem to just get explicitly
zero-initialized (some via xcalloc of a larger struct, so maybe we
should just promise that zero-initialization is always OK).

> >   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.

Ah, right. I was thinking the difference was only on the "value" half
being a pointer versus a struct.

It's nice in the current code that decorations do not incur the extra
cost of storing the oid twice (once in the "struct object", and then
again in the map key).  OTOH, that might well be premature optimization.


      reply	other threads:[~2017-12-15 10:03 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-08  0:14 [PATCH] decorate: clean up and document API Jonathan Tan
2017-12-08  9:55 ` Jeff King
2017-12-11 18:32   ` Jonathan Tan
2017-12-15 10:03     ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).