dead code elimination and sweeping code changes
Matthias Dieter Wallnöfer
mdw at samba.org
Mon Nov 29 14:04:09 MST 2010
Andrew Bartlett wrote:
> Dead code elimination is a difficult issue, and your changes are
> certainly not the right approach to dealing with the issue.
> I fully support Jelmer in his concerns here:
> Just because a compiler suggests something, does not mean is is right,
> correct or proper. Please ask on the list before you make sweeping
> changes like this in future, and revert these changes.
okay, agreed - I won't do dead code elimination anymore without approval.
> The reason that dead code is particularly nasty is that what one
> compiler knows is dead code, another will warn about failing to have a
> termination or not returning a value. Each has different logic to
> determine dead code, and so it is not the case that you actually reduce
> the number of warnings, particularly if you follow an esoteric compiler
> as your guide.
> You as a Samba developer are finally responsible for the quality and
> correctness of any patches you develop. Knowing only that it fixes a
> warning is not the same as knowing the patch is correct. Knowing that
> it does not change the code paths is not even enough to say a patch is
> correct: the warning (now silenced) may actually mean a different
> approach should have be taken elsewhere.
> If you can't positively prove that the patch is correct, please don't
> put it in the tree.
> In particular, the removal of any 'dead' code needs to be done in close
> consultation with the author of the original code: As I've said before,
> the author may have had a different intention - perhaps some *other*
> condition is wrong, and the code isn't meant to be dead. Just like
> adding a cast to fix a compiler warning, the right fix is not always the
> obvious one.
> A more correct approach might be the one I've seen in Heimdal recently:
Yeah, that would be very helpful.
> But that's only starting to be used there, and we would need proper
> header support to even try and have such decorations (It seems to be
> there for Visual C++, heimdal does not yet do anything with this macro
> on Unix compilers).
> In the meantime due to this difficult situation, the code author could
> add add a /* not reached */ if they truly expect the code not to be
> reached, or they may wish to re-factor their code to reach that block.
In my opinion this is a very good chance to discuss this topic - since
sooner or later this would have appeared.
Therefore it would be nice if we all could agree on this policy: mark
intended unreachable code as "/* not reached */" or UNREACHABLE() for
More information about the samba-technical