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, 8 Dec 2017 04:55:11 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Dec 07, 2017 at 04:14:24PM -0800, Jonathan Tan wrote:

> Improve the names of the identifiers in decorate.h, document them, and
> add an example of how to use these functions.
> The example is compiled and run as part of the test suite.
> Signed-off-by: Jonathan Tan <>
> ---
> This patch contains some example code in a test helper.
> There is a discussion at [1] about where example code for hashmap should
> go. For something relatively simple, like decorate, perhaps example code
> isn't necessary; but if we include example code anyway, I think that we
> might as well make it compiled and tested, so this patch contains that
> example code in a test helper.

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.


  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

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

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

  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


  reply	other threads:[~2017-12-08  9:59 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 [this message]
2017-12-11 18:32   ` Jonathan Tan
2017-12-15 10:03     ` Jeff King

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