Re: [PATCH 5/5] strmap: add functions facilitating use as a string->int map

[Elijah Newren <newren@gmail.com>]

On Fri, Aug 21, 2020 at 1:10 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Aug 21, 2020 at 06:52:29PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Although strmap could be used as a string->int map, one either had to
> > allocate an int for every entry and then deallocate later, or one had to
> > do a bunch of casting between (void*) and (intptr_t).
> >
> > Add some special functions that do the casting.  Also, rename put->set
> > for such wrapper functions since 'put' implied there may be some
> > deallocation needed if the string was already found in the map, which
> > isn't the case when we're storing an int value directly in the void*
> > slot instead of using the void* slot as a pointer to data.
>
> I think wrapping this kind of hackery is worth doing.
>
> You'd be able to use put() as usual, wouldn't you? It never deallocates
> the util field, but just returns the old one. And the caller knows that
> it's really an int, and shouldn't be deallocated.

You can use put() as normal, if you don't mind the need to explicitly
throw in a typecast when you use it.  In fact, strintmap_set() does no
more than typecasting the int to void* and otherwise calling
strmap_put().

I initially called that strintmap_put(), but got confused once or
twice and looked up the function definition to make sure there wasn't
some deallocation I needed to handle.  After that, I decided to just
rename to _set() because I thought it'd reduce the chance of myself or
others wondering about that in the future.

>
> > A note on the name: strintmap looks and sounds pretty lame to me, but
> > after trying to come up with something better and having no luck, I
> > figured I'd just go with it for a while and then at some point some
> > better and obvious name would strike me and I could replace it.  Several
> > months later, I still don't have a better name.  Hopefully someone else
> > has one.
>
> strnummap? That's pretty bad, too.
>
> Since the functions all take a raw strmap, you _could_ just do
> "strmap_getint()", etc. But I think you could actually get some
> additional safety by defining a wrapper type:
>
>   struct strintmap {
>           struct strmap strmap;
>   };
>
> It's a bit annoying because you a bunch of pass-through boilerplate for
> stuff like:
>
>   static inline int strintmap_empty(struct strintmap *map)
>   {
>           return strmap_empty(&map->map);
>   }
>
> but it would prevent mistakes like:
>
>   strintmap_incr(&map, "foo", 10);
>   strmap_clear(&map, 1);
>
> which would try to free (void *)10.  I'm not sure if that's worth it or
> not. You'd almost have to be trying to fail to pass "1" for free_util
> there. But I've seen stranger things. :)

I like this idea and the extra safety it provides.  Most of strintmap
is static inline functions anyway, adding a few more wouldn't hurt.