Peter N. Lundblad wrote:
> On Sun, 20 Nov 2005, Erik Huelsmann wrote:
>
>>I get a gcc warning about base_props being used uninitialized in
>>svn_wc__load_props.
Erik, if you "make clean" and compile the whole of Subversion, how many of that
type of warning do you get? Which gcc?
> With gcc 4.0.2 I gets a lot of such warnings all over the code. The topic
> of shutting up warnings has been up for discussion before and some people
> have objected because just setting a value to NULL (or some other bogus
> value) might just hide real bugs. Said more as a general comment to
> current threads. The below doesn't necessarily do that. Problem is that
> different compilers seem to produce this warning in different situations.
> In this case, the warning is bogus, AFAICT.
>
> > I propose this patch to fix it:
>
>
>>Index: subversion/libsvn_wc/props.c
>>===================================================================
>>--- subversion/libsvn_wc/props.c (revision 17455)
>>+++ subversion/libsvn_wc/props.c (working copy)
>>@@ -387,11 +387,14 @@
>> if (base_props_p)
>> *base_props_p = base_props;
>> }
>>+ else
>>+ base_props = NULL;
>>
>> if (props_p)
>> {
>> if (has_propcaching && ! entry->prop_mods && entry->has_props)
>>- *props_p = apr_hash_copy (pool, base_props);
>>+ *props_p = base_props
>>+ ? apr_hash_copy (pool, base_props) : apr_hash_make (pool);
>
> If I'm not missing something (which might well be true, since I wrote this
> code), base_props can never be NULL here. I think it is confusing to add
> code that will never be executed. (An assert is better if that's desired.)
Yes, it's definitely bad to add that second part of the patch if you know
"base_props" is never null there. (Yes, "assert" is better, but hardly
worthwhile here.[1])
The first part, making sure to initialise the variable (which could be done
either as shown or, less obtrusively, at its declaration) is a tough call. My
current feeling is that if there are lots of these cases as you say, then it's
probably not worth adding any code to shut up the warning: the ugliness would
outweigh the benefit. Disable the warning instead; it's one of those warnings
that is only useful for certain styles of coding.
- Julian
[1] "assert" is hardly worthwhile because the pointer is soon dereferenced here
anyway. The potential benefit of "assert" is that it's like documentation: the
programmer declares that the pointer is intended to be non-null. However,
dereferencing a pointer is really just as good a declaration of intent.
There's little reason for a reader to believe that the "assert" is any more
correct and up-to-date than the subsequent code that uses the pointer.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 21 01:56:40 2005