| Message ID | 20210902073631.50062-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG | expand |
On Thu, Sep 02, 2021 at 12:36:31AM -0700, Carlo Marcelo Arenas Belón wrote: > d0da003d5b (use a hashmap to make remotes faster, 2014-07-29) adds > an assert to check that the key added to remote hashmap was unique, > which should never trigger, unless this function is used incorrectly. I'm not sure "unless this function is used incorrectly" is accurate, assuming you mean make_remote() as "this function". The first half of the function checks that the key is not present, and adds it only if that is not true. So there is no way to call make_remote() that will trigger the assertion. It is making sure there is not a bug within make_remote(), but IMHO the primary function is documenting the expectation. I.e., I think this would serve a similar purpose: /* * don't bother checking return; we know there was nothing to * overwrite, since we would have found it above and returned * early. */ hashmap_put_entry(&remotes_hash, ret, ent); But assert()/BUG() is shorter to type _and_ gives a run-time guarantee for cheap, so I think one of those is nicer. > this breaks the build with -DNDEBUG because the assert gets compiled > out and therefore the variable used to check is never used Right, this is the real point of the patch. Compiling with NDEBUG will result in a warning. > remote it and use instead a BUG(), which just like the assert is > not expected to trigger, but will stay put and report regardless of > how the code is compiled. And the solution to switch to a BUG() is good, I think. We just ignore NDEBUG then. But then we do not have to talk about "should never trigger" at all. The point is that we are swapping one assertion mechanism for another, because the new one does not trigger the compiler warning. > @@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len) > remotes[remotes_nr++] = ret; > > hashmap_entry_init(&ret->ent, lookup_entry.hash); > - replaced = hashmap_put_entry(&remotes_hash, ret, ent); > - assert(replaced == NULL); /* no previous entry overwritten */ > + if (hashmap_put_entry(&remotes_hash, ret, ent)) > + BUG("A remote hash collition was detected"); This BUG() text didn't really enlighten me. It's not a hash collision, but rather a duplicate key (you could perhaps call that a collision, but usually "hash collision" refers to an overlap caused by reducing the data to a hash). Something like: BUG("hashmap_put overwrote entry after hashmap_get returned NULL"); That's a bit obscure if a user saw it. But the point is they're not supposed to. The primary audience here is developers who want to understand what is being asserted. -Peff
[dropped Patrick from cc; that address isn't valid anymore, and he's not active in Git development these days] On Thu, Sep 02, 2021 at 04:44:23AM -0400, Jeff King wrote: > > this breaks the build with -DNDEBUG because the assert gets compiled > > out and therefore the variable used to check is never used > > Right, this is the real point of the patch. Compiling with NDEBUG will > result in a warning. Taking all of my suggestions together yields something like: -- >8 -- From: Carlo Marcelo Arenas Belón <carenas@gmail.com> Subject: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG In make_remote(), we store the return value of hashmap_put() and check it using assert(), but don't otherwise use it. If Git is compiled with NDEBUG, then the assert() becomes a noop, and nobody looks at the variable at all. This causes some compilers to produce warnings. Let's switch it instead to a BUG(). This accomplishes the same thing, but is always compiled in (and we don't have to worry about the cost; the check is cheap, and this is not a hot code path). Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- remote.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/remote.c b/remote.c index dfb863d808..40e785da38 100644 --- a/remote.c +++ b/remote.c @@ -135,7 +135,7 @@ static inline void init_remotes_hash(void) static struct remote *make_remote(const char *name, int len) { - struct remote *ret, *replaced; + struct remote *ret; struct remotes_hash_key lookup; struct hashmap_entry lookup_entry, *e; @@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len) remotes[remotes_nr++] = ret; hashmap_entry_init(&ret->ent, lookup_entry.hash); - replaced = hashmap_put_entry(&remotes_hash, ret, ent); - assert(replaced == NULL); /* no previous entry overwritten */ + if (hashmap_put_entry(&remotes_hash, ret, ent)) + BUG("hashmap_put overwrote entry after hashmap_get returned NULL"); return ret; }
Thank you; definitely a big improvement in both accuracy and prose. Carlo
Carlo Arenas <carenas@gmail.com> writes: > Thank you; definitely a big improvement in both accuracy and prose. > > Carlo Thanks, both.
diff --git a/remote.c b/remote.c index dfb863d808..ab9dd35582 100644 --- a/remote.c +++ b/remote.c @@ -135,7 +135,7 @@ static inline void init_remotes_hash(void) static struct remote *make_remote(const char *name, int len) { - struct remote *ret, *replaced; + struct remote *ret; struct remotes_hash_key lookup; struct hashmap_entry lookup_entry, *e; @@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len) remotes[remotes_nr++] = ret; hashmap_entry_init(&ret->ent, lookup_entry.hash); - replaced = hashmap_put_entry(&remotes_hash, ret, ent); - assert(replaced == NULL); /* no previous entry overwritten */ + if (hashmap_put_entry(&remotes_hash, ret, ent)) + BUG("A remote hash collition was detected"); return ret; }
d0da003d5b (use a hashmap to make remotes faster, 2014-07-29) adds an assert to check that the key added to remote hashmap was unique, which should never trigger, unless this function is used incorrectly. this breaks the build with -DNDEBUG because the assert gets compiled out and therefore the variable used to check is never used remote it and use instead a BUG(), which just like the assert is not expected to trigger, but will stay put and report regardless of how the code is compiled. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- remote.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)