{"thread":{"id":"56427","subject":"[PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","startedAt":"2021-09-02T07:38:11Z","lastAt":"2021-09-02T20:13:40Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Jeff King","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434491","messageId":"20210902073631.50062-1-carenas@gmail.com","threadId":"56427","inReplyTo":null,"subject":"[PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T07:36:31Z","receivedAt":"2021-09-02T07:38:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d0da003d5b (use a hashmap to make remotes faster, 2014-07-29) adds\nan assert to check that the key added to remote hashmap was unique,\nwhich should never trigger, unless this function is used incorrectly.\n\nthis breaks the build with -DNDEBUG because the assert gets compiled\nout and therefore the variable used to check is never used\n\nremote it and use instead a BUG(), which just like the assert is\nnot expected to trigger, but will stay put and report regardless of\nhow the code is compiled.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n remote.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex dfb863d808..ab9dd35582 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -135,7 +135,7 @@ static inline void init_remotes_hash(void)\n \n static struct remote *make_remote(const char *name, int len)\n {\n-\tstruct remote *ret, *replaced;\n+\tstruct remote *ret;\n \tstruct remotes_hash_key lookup;\n \tstruct hashmap_entry lookup_entry, *e;\n \n@@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len)\n \tremotes[remotes_nr++] = ret;\n \n \thashmap_entry_init(&ret->ent, lookup_entry.hash);\n-\treplaced = hashmap_put_entry(&remotes_hash, ret, ent);\n-\tassert(replaced == NULL); /* no previous entry overwritten */\n+\tif (hashmap_put_entry(&remotes_hash, ret, ent))\n+\t\tBUG(\"A remote hash collition was detected\");\n \treturn ret;\n }\n \n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434492","messageId":"YTCO56kbtQbODDeK@coredump.intra.peff.net","threadId":"56427","inReplyTo":"20210902073631.50062-1-carenas@gmail.com","subject":"Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T08:44:23Z","receivedAt":"2021-09-02T08:44:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 02, 2021 at 12:36:31AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> d0da003d5b (use a hashmap to make remotes faster, 2014-07-29) adds\n> an assert to check that the key added to remote hashmap was unique,\n> which should never trigger, unless this function is used incorrectly.\n\nI'm not sure \"unless this function is used incorrectly\" is accurate,\nassuming you mean make_remote() as \"this function\". The first half of\nthe function checks that the key is not present, and adds it only if\nthat is not true.\n\nSo there is no way to call make_remote() that will trigger the\nassertion. It is making sure there is not a bug within make_remote(),\nbut IMHO the primary function is documenting the expectation.\n\nI.e., I think this would serve a similar purpose:\n\n /*\n * don't bother checking return; we know there was nothing to\n * overwrite, since we would have found it above and returned\n * early.\n */\n hashmap_put_entry(&remotes_hash, ret, ent);\n\nBut assert()/BUG() is shorter to type _and_ gives a run-time guarantee\nfor cheap, so I think one of those is nicer.\n\n> this breaks the build with -DNDEBUG because the assert gets compiled\n> out and therefore the variable used to check is never used\n\nRight, this is the real point of the patch. Compiling with NDEBUG will\nresult in a warning.\n\n> remote it and use instead a BUG(), which just like the assert is\n> not expected to trigger, but will stay put and report regardless of\n> how the code is compiled.\n\nAnd the solution to switch to a BUG() is good, I think. We just ignore\nNDEBUG then. But then we do not have to talk about \"should never\ntrigger\" at all. The point is that we are swapping one assertion\nmechanism for another, because the new one does not trigger the compiler\nwarning.\n\n> @@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len)\n> \tremotes[remotes_nr++] = ret;\n> \n> \thashmap_entry_init(&ret->ent, lookup_entry.hash);\n> -\treplaced = hashmap_put_entry(&remotes_hash, ret, ent);\n> -\tassert(replaced == NULL); /* no previous entry overwritten */\n> +\tif (hashmap_put_entry(&remotes_hash, ret, ent))\n> +\t\tBUG(\"A remote hash collition was detected\");\n\nThis BUG() text didn't really enlighten me. It's not a hash collision,\nbut rather a duplicate key (you could perhaps call that a collision, but\nusually \"hash collision\" refers to an overlap caused by reducing the\ndata to a hash).\n\nSomething like:\n\n BUG(\"hashmap_put overwrote entry after hashmap_get returned NULL\");\n\nThat's a bit obscure if a user saw it. But the point is they're not\nsupposed to. The primary audience here is developers who want to\nunderstand what is being asserted.\n\n-Peff\n"},{"id":"434493","messageId":"YTCQ5aJXPMTUpKs8@coredump.intra.peff.net","threadId":"56427","inReplyTo":"YTCO56kbtQbODDeK@coredump.intra.peff.net","subject":"Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T08:52:53Z","receivedAt":"2021-09-02T08:52:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[dropped Patrick from cc; that address isn't valid anymore, and he's not\nactive in Git development these days]\n\nOn Thu, Sep 02, 2021 at 04:44:23AM -0400, Jeff King wrote:\n\n> > this breaks the build with -DNDEBUG because the assert gets compiled\n> > out and therefore the variable used to check is never used\n> \n> Right, this is the real point of the patch. Compiling with NDEBUG will\n> result in a warning.\n\nTaking all of my suggestions together yields something like:\n\n-- >8 --\nFrom: Carlo Marcelo Arenas Belón \nSubject: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG\n\nIn make_remote(), we store the return value of hashmap_put() and check\nit using assert(), but don't otherwise use it. If Git is compiled with\nNDEBUG, then the assert() becomes a noop, and nobody looks at the\nvariable at all. This causes some compilers to produce warnings.\n\nLet's switch it instead to a BUG(). This accomplishes the same thing,\nbut is always compiled in (and we don't have to worry about the cost;\nthe check is cheap, and this is not a hot code path).\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Jeff King \n---\n remote.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex dfb863d808..40e785da38 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -135,7 +135,7 @@ static inline void init_remotes_hash(void)\n \n static struct remote *make_remote(const char *name, int len)\n {\n-\tstruct remote *ret, *replaced;\n+\tstruct remote *ret;\n \tstruct remotes_hash_key lookup;\n \tstruct hashmap_entry lookup_entry, *e;\n \n@@ -162,8 +162,8 @@ static struct remote *make_remote(const char *name, int len)\n \tremotes[remotes_nr++] = ret;\n \n \thashmap_entry_init(&ret->ent, lookup_entry.hash);\n-\treplaced = hashmap_put_entry(&remotes_hash, ret, ent);\n-\tassert(replaced == NULL); /* no previous entry overwritten */\n+\tif (hashmap_put_entry(&remotes_hash, ret, ent))\n+\t\tBUG(\"hashmap_put overwrote entry after hashmap_get returned NULL\");\n \treturn ret;\n }\n \n-- \n2.33.0.446.g793367f49a\n\n"},{"id":"434504","messageId":"CAPUEspjMbpbSvT=kVsnFgzTJKtPjU1EBo8cjgvrEqtfbeY4g3Q@mail.gmail.com","threadId":"56427","inReplyTo":"YTCQ5aJXPMTUpKs8@coredump.intra.peff.net","subject":"Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T09:10:36Z","receivedAt":"2021-09-02T09:10:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Thank you; definitely a big improvement in both accuracy and prose.\n\nCarlo\n"},{"id":"434595","messageId":"xmqqa6kuka6s.fsf@gitster.g","threadId":"56427","inReplyTo":"CAPUEspjMbpbSvT=kVsnFgzTJKtPjU1EBo8cjgvrEqtfbeY4g3Q@mail.gmail.com","subject":"Re: [PATCH] remote: avoid -Wunused-but-set-variable in gcc with -DNDEBUG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-02T20:13:31Z","receivedAt":"2021-09-02T20:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> Thank you; definitely a big improvement in both accuracy and prose.\n>\n> Carlo\n\nThanks, both.\n\n"}]}