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

[Jeff King <peff@peff.net>]

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