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. Assignment of a computed value is rather different. So while I agree with your example I still think it's fine to initialize the pointer to NULL in the patch. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/