[PATCH] Initialise hash variable to prevent compiler warnings

[PATCH] Initialise hash variable to prevent compiler warnings

[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

Re: [PATCH] Initialise hash variable to prevent compiler warnings

[Junio C Hamano]

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:

Re: [PATCH] Initialise hash variable to prevent compiler warnings

[Felipe Franciosi]

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:
>
>

Re: [PATCH] Initialise hash variable to prevent compiler warnings

[Junio C Hamano]

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.

Re: [PATCH] Initialise hash variable to prevent compiler warnings

[Felipe Franciosi]

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.

Re: [PATCH] Initialise hash variable to prevent compiler warnings

[Junio C Hamano]

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.