# [PATCH] Initialise hash variable to prevent compiler warnings 6 messages from 2014-10-13 to 2014-10-14. Participants: Felipe Franciosi, Junio C Hamano. Thread: https://gitlist.dev/t/37721 ## Felipe Franciosi, 2014-10-13 14:37 Subject: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: <1413211041-28732-1-git-send-email-felipe@paradoxo.org> URL: https://gitlist.dev/e/1413211041-28732-1-git-send-email-felipe%40paradoxo.org ``` The 'hash' variable in test-hashmap.c is not initialised properly which causes some 'gcc' versions to complain during compilation. Signed-off-by: Felipe Franciosi --- test-hashmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-hashmap.c b/test-hashmap.c index 07aa7ec..cc2891d 100644 --- a/test-hashmap.c +++ b/test-hashmap.c @@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash, char *key, int klen, static unsigned int hash(unsigned int method, unsigned int i, const char *key) { - unsigned int hash; + unsigned int hash = 0; switch (method & 3) { case HASH_METHOD_FNV: -- 1.7.10.4 ``` ## Junio C Hamano, 2014-10-13 20:12 Subject: Re: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: URL: https://gitlist.dev/e/xmqq38ar3f1o.fsf%40gitster.dls.corp.google.com In-Reply-To: <1413211041-28732-1-git-send-email-felipe@paradoxo.org> ``` Felipe Franciosi writes: > The 'hash' variable in test-hashmap.c is not initialised properly > which causes some 'gcc' versions to complain during compilation. FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would have to say that the compiler needs to be fixed. Or insert "default:" just before "case HASH_METHOD_0:" line? I dunno. > > Signed-off-by: Felipe Franciosi > --- > test-hashmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/test-hashmap.c b/test-hashmap.c > index 07aa7ec..cc2891d 100644 > --- a/test-hashmap.c > +++ b/test-hashmap.c > @@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash, char *key, int klen, > > static unsigned int hash(unsigned int method, unsigned int i, const char *key) > { > - unsigned int hash; > + unsigned int hash = 0; > switch (method & 3) > { > case HASH_METHOD_FNV: ``` ## Felipe Franciosi, 2014-10-13 21:55 Subject: Re: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: URL: https://gitlist.dev/e/CANxchRRtd03LvjbWO7d45EE-GB_yTZjEhcJccZW_Y3mgDpENrQ%40mail.gmail.com In-Reply-To: ``` On Mon, Oct 13, 2014 at 10:53 PM, Felipe Franciosi wrote: > > On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote: >> >> Felipe Franciosi writes: >> >> > The 'hash' variable in test-hashmap.c is not initialised properly >> > which causes some 'gcc' versions to complain during compilation. >> >> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would >> have to say that the compiler needs to be fixed. >> >> Or insert "default:" just before "case HASH_METHOD_0:" line? >> >> I dunno. Hmm... The "default:" would work, but is it really that bad to initialise a local variable in this case? In any case, the compilation warning is annoying. Do you prefer the default or the initialisation? Cheers, F. > > > Hmm... The "default:" would work, but is it really that bad to initialise a > local variable in this case? > > In any case, the compilation warning is annoying. Do you prefer the default > or the initialisation? > > Cheers, > F. > >> >> > >> > Signed-off-by: Felipe Franciosi >> > --- >> > test-hashmap.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/test-hashmap.c b/test-hashmap.c >> > index 07aa7ec..cc2891d 100644 >> > --- a/test-hashmap.c >> > +++ b/test-hashmap.c >> > @@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash, >> > char *key, int klen, >> > >> > static unsigned int hash(unsigned int method, unsigned int i, const >> > char *key) >> > { >> > - unsigned int hash; >> > + unsigned int hash = 0; >> > switch (method & 3) >> > { >> > case HASH_METHOD_FNV: > > ``` ## Junio C Hamano, 2014-10-14 01:13 Subject: Re: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: URL: https://gitlist.dev/e/CAPc5daWmHe-EbgO-wjy9UsodTFERa1vpz-SEDF-UM_eLm7CuCw%40mail.gmail.com In-Reply-To: ``` On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi wrote: > > On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote: >> >> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would >> have to say that the compiler needs to be fixed. >> >> Or insert "default:" just before "case HASH_METHOD_0:" line? >> >> I dunno. > > Hmm... The "default:" would work, but is it really that bad to initialise a > local variable in this case? > > In any case, the compilation warning is annoying. Do you prefer the default > or the initialisation? If I really had to choose between the two, adding a useless initialization would be the less harmful choice. Adding a meaningless "default:" robs another chance from the compilers to diagnose a future breakage we might add (namely, we may extend methods and forget to write a corresponding case arm for the new method value, which a smart compiler can and do diagnose as a switch that does not handle all the possible values. Thanks. ``` ## Felipe Franciosi, 2014-10-14 11:44 Subject: Re: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: URL: https://gitlist.dev/e/CANxchRQ%3DkR4N5s5S8eSJy2de__BydWDgowCNb6QQW6JZyvQAMw%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano wrote: > On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi wrote: >> >> On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote: >>> >>> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would >>> have to say that the compiler needs to be fixed. >>> >>> Or insert "default:" just before "case HASH_METHOD_0:" line? >>> >>> I dunno. >> >> Hmm... The "default:" would work, but is it really that bad to initialise a >> local variable in this case? >> >> In any case, the compilation warning is annoying. Do you prefer the default >> or the initialisation? > > If I really had to choose between the two, adding a useless initialization > would be the less harmful choice. Adding a meaningless "default:" robs > another chance from the compilers to diagnose a future breakage we > might add (namely, we may extend methods and forget to write a > corresponding case arm for the new method value, which a smart > compiler can and do diagnose as a switch that does not handle > all the possible values. > > Thanks. I see your point; the code is correct today because it covers all cases. Nevertheless, some versions of gcc (the one I used was 4.1.2 from CentOS 5.10 -- haven't tested others) might generate an annoying warning. Noting that, I also like my code to compile as cleanly as possible in all environments that it might be used. Being a bit defensive in that sense and initialising local variables is what I would do. On top of that (and putting the compiler flaw aside for a moment), having it sensibly initialised is another way of protecting the code against errors introduced in the future. What do you think? Cheers, F. ``` ## Junio C Hamano, 2014-10-14 16:41 Subject: Re: [PATCH] Initialise hash variable to prevent compiler warnings Message-ID: URL: https://gitlist.dev/e/CAPc5daXFuuxW%3DPSB_wKw3CcUGiM1sBPZXQ4UJoqHd1ZOPh7F8w%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 14, 2014 at 4:44 AM, Felipe Franciosi wrote: > On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano wrote: >> >> If I really had to choose between the two, adding a useless initialization >> would be the less harmful choice. Adding a meaningless "default:" robs >> ... > Being a bit defensive in that > sense and initialising local variables is what I would do. On top of > that (and putting the compiler flaw aside for a moment), having it > sensibly initialised is another way of protecting the code against > errors introduced in the future. That is a false sense of safety. You will not know if the new method introduced in the future would behave sensibly if the variable is left in a state the blanket initialization created, so setting it to 0 upfront is not really being defensive; you would rob compilers a chance to notice something is amiss in the future code with the initialization, just like a "default:" would. We need to accept that both are not about being defensive but are ways to work around stupid compilers from reporting false positives. I am not saying that we should not do a work around. I am only saying that it is wrong to try selling such a work around as a defensive good practice, which is not. > What do you think? Again, if I really had to choose between the two, adding a useless initialization would be the less harmful choice, as the other one has an extra downside. ```