[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] initialize some variables in test to prevent -Wmaybe-uninitialized

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 18 Nov 2013 13:39:58 +0000 (GMT)

Daniel Shahaf wrote:

> Andreas Stieger wrote on Sat, Nov 16, 2013 at 00:08:40 +0000:
>> subversion/tests/libsvn_fs_base/fs-base-test.c:239:6: warning: 'present'
>> may be used uninitialized in this function [-Wmaybe-uninitialized] et
>> al. Initializing to boolean value that would fail the test if the
>> reference wasn't changed in called function at all.
>>
>> [[[
>> * subversion/tests/libsvn_fs/fs-test.c
>>   (check_entry_present, check_entry_absent),
>>   subversion/tests/libsvn_fs_base/fs-base-test.c
[...]
>>   (check_entry_present, check_entry_absent, check_id_present,
>>   check_id_absent): initialize variable present to silence
>>   warning -Wmaybe-uninitialized
>> ]]]

This looks to me like a false warning. I
can see by inspection that check_entry() always initializes this variable or returns an
error, and in the latter case the variable isn't used.

As check_entry() is a local function, and is part of the test rather
 than part of the system under test, it's not necessary to be so
careful: we can trust it just as much as we can trust its callers. So
the only reason to change anything is to silence the warning. In general
 I don't want to add extra code just to get rid of spurious warnings.
However, in this case I have no objection to the =FALSE / =TRUE variant.

>> Andreas
>
>> Index: subversion/tests/libsvn_fs/fs-test.c
>> ===================================================================
[...]
>> -  svn_boolean_t present;
>> +  svn_boolean_t present = false;
>>     SVN_ERR(check_entry(root, path, name, &present, pool));
>>  
>>     if (! present)
[...]
>
> Personally I'd consider a tristate here, to allow the initialization to
> be the same regardless of the sense of the condition in the 'if'.

Ugh, that's OTT. The function's result is already tri-state: it yields TRUE or FALSE or returns an error.

> If you keep it a boolean, you should use FALSE and TRUE instead; those
> are in C89, and 'false'/'true' aren't.
>
> If you make either of those two changes, +1 to commit.  (If further
> changes are needed, they can be made in subsequent commits.)

- Julian
Received on 2013-11-18 14:40:38 CET

This is an archived mail posted to the Subversion Dev mailing list.