Re: [PATCH 5/5] strmap: add functions facilitating use as a string->int map
- From
Elijah Newren <newren@gmail.com>
- Date
- Aug 21, 2020, 20:51 UTC
- Message-ID
- <CABPp-BFi7JSkkVSLkS9Kb5fMJ3xM3CjM7_ZsvJZMv=XgQgb6Lw@mail.gmail.com>
- In-Reply-To
- <20200821201034.GG1165@coredump.intra.peff.net>
On Fri, Aug 21, 2020 at 1:10 PM Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> > 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.
Show 34 quoted lines
>
> > 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.