Daniel Veillard wrote:
> On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote:
>> Jim Meyering wrote:
>> > Daniel P. Berrange wrote:
>> > ...
>> >>> Actually I did that first, but then un-did it in favor
>> >>> of the change above. Why? because that initialization could
>> >>> mask a failure to initialize in a new case.
>> >>>
>> >>> With per-case initialization, we'd detect the bug at
>> >>> compile/static-analysis time. With the up-front unconditional
>> >>> initialization, we cannot, and would have to rely on testing to find it.
>> >>
>> >> It is a tradeoff, but I still prefer the initialization at time of
>> >> declaration as a safety net, and we do use this pattern pretty much
>> >> everywhere
>> >
>> > Ok. adjusted
>>
>> Actually, I will now try to convince you that we should
>> do it the other way. Not only is it better to have the compiler
>> tell us about this problem, but if ever someone were to add the
>> definition that seems to be missing in the default case, clang
>> would report the preceding initialization (the one you prefer)
>> as a "dead store" error.
>>
> [...]
>>
>> And in every "case" of that switch, there is an assignment to "group".
>> That renders the preceding assignment useless, and hence clang calls it
>> a dead initialization.
>>
>> Just to show you that they're not all so ambiguous, node_device_conf.c:116
>> has a "dead initialization" that is obviously worth fixing:
>>
>> virNodeDevCapsDefPtr caps = def->caps;
>> ...
>> for (caps = def->caps; caps; caps = caps->next) {
>>
>> Posting that separately.
>
> I must admit I don't see a NULL initialization of a pointer for safety
> when entering a routine as a bad thing, actually this could be
> considered a standard feature in any highlevel language.
Then perhaps I didn't explain well.
Here's a small example:
int
foo (void)
{
int ret = 0;
switch (f())
{
case 0:
do_something ();
ret = -1;
break;
case 1:
do_something_else ();
ret = -1;
break;
default:
ret = -2;
break;
}
return ret;
}
In that function, the initialization of "ret" is officially unnecessary.
Or, make it more like the code in question, initializing to
the value used in the "default" case:
int
foo (void)
{
int ret = -2;
switch (f())
{
case 0:
do_something ();
ret = -1;
break;
case 1:
do_something_else ();
ret = -1;
break;
default:
break;
}
return ret;
}
However, Dan argued that he wants to be sure "ret" is defined, in case
someone else (one of the case stmts) forgets. However, that's precisely
the case in which you do *not* want there to be an overarching
definition, because with that "extra", initial assignment, the compiler
cannot warn you if you add a case like this:
case 2:
// ...do things, but forget to set "ret"
break;
However, if you merely declare "ret", with no initial value,
adding that erroneous "case 2" would provoke a warning
about "ret" being used (via the return) undefined.
I prefer to use the compiler as a safety net, whenever possible.
I.e., imho, it is clear that this toy function should be written
like this, with no initial assignment to "ret":
int
foo (void)
{
int ret;
switch (f())
{
case 0:
do_something ();
ret = -1;
break;
case 1:
do_something_else ();
ret = -1;
break;
default:
ret = -2;
break;
}
return ret;
}
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list