Re: [PATCH v2] oidmap: map with OID as key

From: Jeff King <peff@peff.net>
Date: Oct 3, 2017, 06:31 UTC
Message-ID: <20171003063119.iht5kl7zsiuxpaqz@sigill.intra.peff.net>
In-Reply-To: <20171002234848.GG5189@google.com>

On Mon, Oct 02, 2017 at 04:48:48PM -0700, Brandon Williams wrote:

Show 14 quoted lines

> > Some replies to v1 [1] [2] seem to indicate that simpler non-duplicated
> > code should be preferred over optimizing away the storage of the 4-byte
> > hash code, and I have no objection to that, so I have updated this code
> > to be a thin wrapper over hashmap with the 4-byte overhead.
> > 
> > After this patch, if the 4-byte overhead is found to be too much, we can
> > migrate to something similar to v1 relatively easily.
> > 
> > I decided not to go with the util pointer method because we will not be
> > able to migrate away from it so easily if need be.
> 
> This makes me a bit sad because I tend to lean more towards making
> things simpler.  I'm still a supporter of the 'util' pointer but I
> understand why we would choose otherwise.

Right, I kind of wonder if this has fallen into an uncanny value where we have this almost-hashmap infrastructure, but the end result is not significantly easier to use than a plain-old hashmap.

I.e., it looks like you still have to declare something like:

  struct my_data {
        struct oidmap_entry oid;
	int value; /* mapping to an int */
  };

and handle the allocation of the entry yourself. If we instead just adding an oidhash() and oidcmpfn(), then callers could those directly.

The invocations are a _little_ longer with a raw hashmap, but not much (as you can see from the actual oidmap implementation, and the changes to oidset).

I dunno. I'm not against it per se. The API _is_ a little nicer, but I just wonder if there's a downside to even the thin wrapper, in that callers are no longer free to use other parts of the hashmap API. If I saw some converted callers I might be more convinced.

-Peff