dead code elimination and sweeping code changes

[Andrew Bartlett]

On Mon, 2010-11-29 at 20:40 +0100, Matthias Dieter Wallnöfer wrote:
> Jelmer,
> 
> Jelmer Vernooij wrote:
> > Then please don't say you don't consider yourself responsible (perhaps
> > there is some language confusion here?).
> >    
> Probably I've not expressed it correctly.
> It was like this: since the Solaris "cc" compiler blamed about this I 
> thought that it would be convenient to quiet these warnings. Exactly 
> this was the intention.
> And I really think that it's not a fault to fix warnings (naturally in a 
> convenient and acceptable manner) if the behaviour doesn't change. And 
> exactly here we have such a case.

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. 

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:

UNREACHABLE(statement)

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. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101130/ed67e85d/attachment.pgp>