{"thread":{"id":"37721","subject":"[PATCH] Initialise hash variable to prevent compiler warnings","startedAt":"2014-10-13T14:37:21Z","lastAt":"2014-10-14T16:41:31Z","messageCount":6,"participants":["Felipe Franciosi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250538","messageId":"1413211041-28732-1-git-send-email-felipe@paradoxo.org","threadId":"37721","inReplyTo":null,"subject":"[PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Felipe Franciosi","fromEmail":"felipe@paradoxo.org","sentAt":"2014-10-13T14:37:21Z","receivedAt":"2014-10-13T14:37:21Z","isPatch":true,"sender":{"key":"felipe@paradoxo.org","avatar":"https://gravatar.com/avatar/37a42a13ccbd0ac2a9cae8c292b834c94b3311afd908013743c95d9525425479?d=mp&s=160"},"body":"The 'hash' variable in test-hashmap.c is not initialised properly\nwhich causes some 'gcc' versions to complain during compilation.\n\nSigned-off-by: Felipe Franciosi \n---\n test-hashmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-hashmap.c b/test-hashmap.c\nindex 07aa7ec..cc2891d 100644\n--- a/test-hashmap.c\n+++ b/test-hashmap.c\n@@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash, char *key, int klen,\n \n static unsigned int hash(unsigned int method, unsigned int i, const char *key)\n {\n-\tunsigned int hash;\n+\tunsigned int hash = 0;\n \tswitch (method & 3)\n \t{\n \tcase HASH_METHOD_FNV:\n-- \n1.7.10.4\n"},{"id":"250573","messageId":"xmqq38ar3f1o.fsf@gitster.dls.corp.google.com","threadId":"37721","inReplyTo":"1413211041-28732-1-git-send-email-felipe@paradoxo.org","subject":"Re: [PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-13T20:12:03Z","receivedAt":"2014-10-13T20:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Franciosi writes:\n\n> The 'hash' variable in test-hashmap.c is not initialised properly\n> which causes some 'gcc' versions to complain during compilation.\n\nFNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would\nhave to say that the compiler needs to be fixed.\n\nOr insert \"default:\" just before \"case HASH_METHOD_0:\" line?\n\nI dunno.\n\n>\n> Signed-off-by: Felipe Franciosi \n> ---\n> test-hashmap.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/test-hashmap.c b/test-hashmap.c\n> index 07aa7ec..cc2891d 100644\n> --- a/test-hashmap.c\n> +++ b/test-hashmap.c\n> @@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash, char *key, int klen,\n> \n> static unsigned int hash(unsigned int method, unsigned int i, const char *key)\n> {\n> -\tunsigned int hash;\n> +\tunsigned int hash = 0;\n> \tswitch (method & 3)\n> \t{\n> \tcase HASH_METHOD_FNV:\n"},{"id":"250583","messageId":"CANxchRRtd03LvjbWO7d45EE-GB_yTZjEhcJccZW_Y3mgDpENrQ@mail.gmail.com","threadId":"37721","inReplyTo":"CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com","subject":"Re: [PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Felipe Franciosi","fromEmail":"felipe@paradoxo.org","sentAt":"2014-10-13T21:55:01Z","receivedAt":"2014-10-13T21:55:01Z","isPatch":true,"sender":{"key":"felipe@paradoxo.org","avatar":"https://gravatar.com/avatar/37a42a13ccbd0ac2a9cae8c292b834c94b3311afd908013743c95d9525425479?d=mp&s=160"},"body":"On Mon, Oct 13, 2014 at 10:53 PM, Felipe Franciosi wrote:\n>\n> On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote:\n>>\n>> Felipe Franciosi writes:\n>>\n>> > The 'hash' variable in test-hashmap.c is not initialised properly\n>> > which causes some 'gcc' versions to complain during compilation.\n>>\n>> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would\n>> have to say that the compiler needs to be fixed.\n>>\n>> Or insert \"default:\" just before \"case HASH_METHOD_0:\" line?\n>>\n>> I dunno.\n\nHmm... The \"default:\" would work, but is it really that bad to\ninitialise a local variable in this case?\n\nIn any case, the compilation warning is annoying. Do you prefer the\ndefault or the initialisation?\n\nCheers,\nF.\n\n>\n>\n> Hmm... The \"default:\" would work, but is it really that bad to initialise a\n> local variable in this case?\n>\n> In any case, the compilation warning is annoying. Do you prefer the default\n> or the initialisation?\n>\n> Cheers,\n> F.\n>\n>>\n>> >\n>> > Signed-off-by: Felipe Franciosi \n>> > ---\n>> > test-hashmap.c | 2 +-\n>> > 1 file changed, 1 insertion(+), 1 deletion(-)\n>> >\n>> > diff --git a/test-hashmap.c b/test-hashmap.c\n>> > index 07aa7ec..cc2891d 100644\n>> > --- a/test-hashmap.c\n>> > +++ b/test-hashmap.c\n>> > @@ -47,7 +47,7 @@ static struct test_entry *alloc_test_entry(int hash,\n>> > char *key, int klen,\n>> >\n>> > static unsigned int hash(unsigned int method, unsigned int i, const\n>> > char *key)\n>> > {\n>> > - unsigned int hash;\n>> > + unsigned int hash = 0;\n>> > switch (method & 3)\n>> > {\n>> > case HASH_METHOD_FNV:\n>\n>\n"},{"id":"250596","messageId":"CAPc5daWmHe-EbgO-wjy9UsodTFERa1vpz-SEDF-UM_eLm7CuCw@mail.gmail.com","threadId":"37721","inReplyTo":"CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com","subject":"Re: [PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-14T01:13:31Z","receivedAt":"2014-10-14T01:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi wrote:\n>\n> On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote:\n>>\n>> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would\n>> have to say that the compiler needs to be fixed.\n>>\n>> Or insert \"default:\" just before \"case HASH_METHOD_0:\" line?\n>>\n>> I dunno.\n>\n> Hmm... The \"default:\" would work, but is it really that bad to initialise a\n> local variable in this case?\n>\n> In any case, the compilation warning is annoying. Do you prefer the default\n> or the initialisation?\n\nIf I really had to choose between the two, adding a useless initialization\nwould be the less harmful choice. Adding a meaningless \"default:\" robs\nanother chance from the compilers to diagnose a future breakage we\nmight add (namely, we may extend methods and forget to write a\ncorresponding case arm for the new method value, which a smart\ncompiler can and do diagnose as a switch that does not handle\nall the possible values.\n\nThanks.\n"},{"id":"250604","messageId":"CANxchRQ=kR4N5s5S8eSJy2de__BydWDgowCNb6QQW6JZyvQAMw@mail.gmail.com","threadId":"37721","inReplyTo":"CAPc5daWmHe-EbgO-wjy9UsodTFERa1vpz-SEDF-UM_eLm7CuCw@mail.gmail.com","subject":"Re: [PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Felipe Franciosi","fromEmail":"felipe@paradoxo.org","sentAt":"2014-10-14T11:44:17Z","receivedAt":"2014-10-14T11:44:17Z","isPatch":true,"sender":{"key":"felipe@paradoxo.org","avatar":"https://gravatar.com/avatar/37a42a13ccbd0ac2a9cae8c292b834c94b3311afd908013743c95d9525425479?d=mp&s=160"},"body":"On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano wrote:\n> On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi wrote:\n>>\n>> On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano wrote:\n>>>\n>>> FNV/I/IDIV10/0 covers all the possibilities of (method & 3), I would\n>>> have to say that the compiler needs to be fixed.\n>>>\n>>> Or insert \"default:\" just before \"case HASH_METHOD_0:\" line?\n>>>\n>>> I dunno.\n>>\n>> Hmm... The \"default:\" would work, but is it really that bad to initialise a\n>> local variable in this case?\n>>\n>> In any case, the compilation warning is annoying. Do you prefer the default\n>> or the initialisation?\n>\n> If I really had to choose between the two, adding a useless initialization\n> would be the less harmful choice. Adding a meaningless \"default:\" robs\n> another chance from the compilers to diagnose a future breakage we\n> might add (namely, we may extend methods and forget to write a\n> corresponding case arm for the new method value, which a smart\n> compiler can and do diagnose as a switch that does not handle\n> all the possible values.\n>\n> Thanks.\n\nI see your point; the code is correct today because it covers all\ncases. Nevertheless, some versions of gcc (the one I used was 4.1.2\nfrom CentOS 5.10 -- haven't tested others) might generate an annoying\nwarning.\n\nNoting that, I also like my code to compile as cleanly as possible in\nall environments that it might be used. Being a bit defensive in that\nsense and initialising local variables is what I would do. On top of\nthat (and putting the compiler flaw aside for a moment), having it\nsensibly initialised is another way of protecting the code against\nerrors introduced in the future.\n\nWhat do you think?\n\nCheers,\nF.\n"},{"id":"250613","messageId":"CAPc5daXFuuxW=PSB_wKw3CcUGiM1sBPZXQ4UJoqHd1ZOPh7F8w@mail.gmail.com","threadId":"37721","inReplyTo":"CANxchRQ=kR4N5s5S8eSJy2de__BydWDgowCNb6QQW6JZyvQAMw@mail.gmail.com","subject":"Re: [PATCH] Initialise hash variable to prevent compiler warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-14T16:41:31Z","receivedAt":"2014-10-14T16:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Oct 14, 2014 at 4:44 AM, Felipe Franciosi wrote:\n> On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano wrote:\n>>\n>> If I really had to choose between the two, adding a useless initialization\n>> would be the less harmful choice. Adding a meaningless \"default:\" robs\n>> ...\n> Being a bit defensive in that\n> sense and initialising local variables is what I would do. On top of\n> that (and putting the compiler flaw aside for a moment), having it\n> sensibly initialised is another way of protecting the code against\n> errors introduced in the future.\n\nThat is a false sense of safety. You will not know if the new method\nintroduced in the future would behave sensibly if the variable is left\nin a state the blanket initialization created, so setting it to 0 upfront\nis not really being defensive; you would rob compilers a chance\nto notice something is amiss in the future code with the initialization,\njust like a \"default:\" would. We need to accept that both are not about\nbeing defensive but are ways to work around stupid compilers from\nreporting false positives.\n\nI am not saying that we should not do a work around. I am only\nsaying that it is wrong to try selling such a work around as a defensive\ngood practice, which is not.\n\n> What do you think?\n\nAgain, if I really had to choose between the two, adding a useless\ninitialization would be the less harmful choice, as the other one has\nan extra downside.\n"}]}