Re: [PATCH v1 3/4] hashmap: add simplified hashmap_get_from_hash() API

[Junio C Hamano <gitster@pobox.com>]

Karsten Blees <karsten.blees@gmail.com> writes:

> Hashmap entries are typically looked up by just a key. The hashmap_get()
> API expects an initialized entry structure instead, to support compound
> keys. This flexibility is currently only needed by find_dir_entry() in
> name-hash.c (and compat/win32/fscache.c in the msysgit fork). All other
> (currently five) call sites of hashmap_get() have to set up a near emtpy

s/emtpy/empty/;

> entry structure, resulting in duplicate code like this:
>
>   struct hashmap_entry keyentry;
>   hashmap_entry_init(&keyentry, hash(key));
>   return hashmap_get(map, &keyentry, key);
>
> Add a hashmap_get_from_hash() API that allows hashmap lookups by just
> specifying the key and its hash code, i.e.:
>
>   return hashmap_get_from_hash(map, hash(key), key);

While I think the *_get_from_hash() is an improvement over the
three-line sequence, I find it somewhat strange that callers of it
still must compute the hash code themselves, instead of letting the
hashmap itself to apply the user supplied hash function to the key
to derive it.  After all, the map must know how to compare two
entries via a user-supplied cmpfn given at the map initialization
time, and the algorithm to derive the hash code falls into the same
category, in the sense that both must stay the same during the
lifetime of a hashmap, no?  Unless there is a situation where you
need to be able to initialize a hashmap_entry without knowing which
hashmap the entry will eventually be stored, it feels dubious API
that exposes to the outside callers hashmap_entry_init() that takes
the hash code without taking the hashmap itself.

In other words, why isn't hashmap_get() more like this:

        void *hashmap_get(const struct hashmap *map, const void *key)
        {
                struct hashmap_entry keyentry;
                hashmap_entry_init(&keyentry, map->hash(key));
                return *find_entry_ptr(map, &keyentry, key);
        }

with hashmap_entry_init() purely a static helper in hashmap.c?