* [PATCH] Initialise hash variable to prevent compiler warnings @ 2014-10-13 14:37 Felipe Franciosi 2014-10-13 20:12 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Felipe Franciosi @ 2014-10-13 14:37 UTC (permalink / raw) To: git; +Cc: Felipe Franciosi 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 <felipe@paradoxo.org> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Initialise hash variable to prevent compiler warnings 2014-10-13 14:37 [PATCH] Initialise hash variable to prevent compiler warnings Felipe Franciosi @ 2014-10-13 20:12 ` Junio C Hamano [not found] ` <CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2014-10-13 20:12 UTC (permalink / raw) To: Felipe Franciosi; +Cc: git Felipe Franciosi <felipe@paradoxo.org> 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 <felipe@paradoxo.org> > --- > 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: ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com>]
* Re: [PATCH] Initialise hash variable to prevent compiler warnings [not found] ` <CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com> @ 2014-10-13 21:55 ` Felipe Franciosi 2014-10-14 1:13 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Felipe Franciosi @ 2014-10-13 21:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Oct 13, 2014 at 10:53 PM, Felipe Franciosi <felipe@paradoxo.org> wrote: > > On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano <gitster@pobox.com> wrote: >> >> Felipe Franciosi <felipe@paradoxo.org> 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 <felipe@paradoxo.org> >> > --- >> > 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: > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Initialise hash variable to prevent compiler warnings [not found] ` <CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com> 2014-10-13 21:55 ` Felipe Franciosi @ 2014-10-14 1:13 ` Junio C Hamano 2014-10-14 11:44 ` Felipe Franciosi 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2014-10-14 1:13 UTC (permalink / raw) To: Felipe Franciosi; +Cc: Git Mailing List On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi <felipe@paradoxo.org> wrote: > > On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano <gitster@pobox.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Initialise hash variable to prevent compiler warnings 2014-10-14 1:13 ` Junio C Hamano @ 2014-10-14 11:44 ` Felipe Franciosi 2014-10-14 16:41 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Felipe Franciosi @ 2014-10-14 11:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano <gitster@pobox.com> wrote: > On Mon, Oct 13, 2014 at 2:53 PM, Felipe Franciosi <felipe@paradoxo.org> wrote: >> >> On Mon, Oct 13, 2014 at 9:12 PM, Junio C Hamano <gitster@pobox.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Initialise hash variable to prevent compiler warnings 2014-10-14 11:44 ` Felipe Franciosi @ 2014-10-14 16:41 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2014-10-14 16:41 UTC (permalink / raw) To: Felipe Franciosi; +Cc: Git Mailing List On Tue, Oct 14, 2014 at 4:44 AM, Felipe Franciosi <felipe@paradoxo.org> wrote: > On Tue, Oct 14, 2014 at 2:13 AM, Junio C Hamano <gitster@pobox.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-14 16:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-13 14:37 [PATCH] Initialise hash variable to prevent compiler warnings Felipe Franciosi
2014-10-13 20:12 ` Junio C Hamano
[not found] ` <CANxchRS1mGapb77hc9Ywqj_-8UeexSAWK4UK9y9M76pvoN-Yeg@mail.gmail.com>
2014-10-13 21:55 ` Felipe Franciosi
2014-10-14 1:13 ` Junio C Hamano
2014-10-14 11:44 ` Felipe Franciosi
2014-10-14 16:41 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).