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.
This isn't a dead store though - we are setting the default return value for cases where there's no explicit return value set. It would only become a dead store if we also added the redundant 'ret = NULL' initialization that your initial patch did.
In fact, now that I'm looking at what clang calls dead initializations, I see just such a case in mdns.c:
These are different scenarios dealing with local use variables only, rather tha default return values as the above case. Removing these redundant inits makes sense for mdns/nodedev. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|