Jim Meyering wrote: 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. In fact, now that I'm looking at what clang calls dead initializations, I see just such a case in mdns.c: static void libvirtd_mdns_client_callback(AvahiClient *c, AvahiClientState state, void *userdata) { ... struct libvirtd_mdns_group *group = mdns->group; ... switch (state) { ... } 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.

On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote: 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. 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 :|

Daniel Veillard wrote: 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; }