dead code elimination and sweeping code changes

[Matthias Dieter Wallnöfer]

Hi Andrew,

Andrew Bartlett wrote:
> Matthias,
>
> 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.
>    
Okay
> 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:
>
> UNREACHABLE(statement)
>    
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.
>    
Okay.

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 
safety purposes.

Cheers,
Matthias