[PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[Carlo Marcelo Arenas Belón]

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

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;
 }
 
-- 
2.33.0.481.g26d3bed244

Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[Jeff King]

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

Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[Jeff King]

[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;
 }
 
-- 
2.33.0.446.g793367f49a

Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[Carlo Arenas]

Thank you; definitely a big improvement in both accuracy and prose.

Carlo

Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> Thank you; definitely a big improvement in both accuracy and prose.
>
> Carlo

Thanks, both.