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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 4, 2017, 00:29 UTC
Message-ID: <CAGf8dgKtwdT912CaARdjYnSAd2m7mmdOzM=OGCUupw80snrd_w@mail.gmail.com>
In-Reply-To: <20171003063119.iht5kl7zsiuxpaqz@sigill.intra.peff.net>

On Mon, Oct 2, 2017 at 11:31 PM, Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

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

I thought of something like that, but it seems that you have to
remember quite a few things:
- your entry must have "struct oidmap_entry" at the start, not "struct
hashmap_entry"
- initialize your hashmap with oidcmpfn()
- when getting, hashmap_get_from_hash(map, oidhash(&oid), &oid) (and
oid might be longer e.g. ref->old_oid)

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

About the invocation of hashmap_get_from_hash(), I felt that it would get annoying quickly enough that I would want an oidmap_get(const struct hashmap *, const struct object_id *) but it might be strange that the "get" method is named differently from the rest. If we tolerate oidmap_get(), and tolerate the fact that the user must both declare "struct oidmap_entry" instead of "struct hashmap_entry" and initialize the hashmap with oidcmpfn() (so that the invocation to hashmap_get_from_hash() within oidmap_get() sends the correct keydata), we can avoid the thin wrapper issue where callers can no longer use other methods of hashmap. At this point I decided that I prefer the thin wrapper, but the "light touch" (struct oidmap_entry, oidcmpfn(), oidmap_get() only) still better than the status quo.