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

Commit Message

Johannes Schindelin via GitGitGadget Aug. 21, 2020, 6:52 p.m. UTC

From: Elijah Newren <[e-mail]>

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.

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
 strmap.c | 11 +++++++++++
 strmap.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

Comments

Jeff King Aug. 21, 2020, 8:10 p.m. UTC | #1

On Fri, Aug 21, 2020 at 06:52:29PM +0000, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
> 
> 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.

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

-Peff

Elijah Newren Aug. 21, 2020, 8:51 p.m. UTC | #2

On Fri, Aug 21, 2020 at 1:10 PM Jeff King <[e-mail]> wrote:
>
> On Fri, Aug 21, 2020 at 06:52:29PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <[e-mail]>
> >
> > 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.

Jeff King Aug. 21, 2020, 9:05 p.m. UTC | #3

On Fri, Aug 21, 2020 at 01:51:57PM -0700, Elijah Newren wrote:

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

Yeah, I think hiding the type-casting is worth it alone. I was just
confused by your remark.

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

Yeah, I'd agree that is a much better name. Since there's an "incr",
having a specific "set" makes it clear that we're overwriting.

> >   struct strintmap {
> >           struct strmap strmap;
> >   };
> [...]
> 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.

OK. Then I guess we can't cheat our way out of picking a name with
strmap_getint(). :)

-Peff

Patch

diff --git a/strmap.c b/strmap.c
index 03eb6af45d..cbb99f4030 100644
--- a/strmap.c
+++ b/strmap.c
@@ -113,3 +113,14 @@  void strmap_remove(struct strmap *map, const char *str, int free_util)
 		free(ret->item.util);
 	free(ret);
 }
+
+void strintmap_incr(struct strmap *map, const char *str, intptr_t amt)
+{
+	struct str_entry *entry = find_str_entry(map, str);
+	if (entry) {
+		intptr_t *whence = (intptr_t*)&entry->item.util;
+		*whence += amt;
+	}
+	else
+		strintmap_set(map, str, amt);
+}
diff --git a/strmap.h b/strmap.h
index 28a98c5a4b..5d9dd3ef58 100644
--- a/strmap.h
+++ b/strmap.h
@@ -88,4 +88,36 @@  static inline unsigned int strmap_get_size(struct strmap *map)
 		var = hashmap_iter_next_entry_offset(iter, \
 						OFFSETOF_VAR(var, ent)))
 
+/*
+ * Helper functions for using strmap as map of string -> int, using the void*
+ * field to store the int instead of allocating an int and having the void*
+ * member point to the allocated int.
+ */
+
+static inline int strintmap_get(struct strmap *map, const char *str,
+				int default_value)
+{
+	struct string_list_item *result = strmap_get_item(map, str);
+	if (!result)
+		return default_value;
+	return (intptr_t)result->util;
+}
+
+static inline void strintmap_set(struct strmap *map, const char *str, intptr_t v)
+{
+	strmap_put(map, str, (void *)v);
+}
+
+void strintmap_incr(struct strmap *map, const char *str, intptr_t amt);
+
+static inline void strintmap_clear(struct strmap *map)
+{
+	strmap_clear(map, 0);
+}
+
+static inline void strintmap_free(struct strmap *map)
+{
+	strmap_free(map, 0);
+}
+
 #endif /* STRMAP_H */