On Wed, 24 Nov 2021, Michael Matz wrote:
> Hello,
>
> On Wed, 24 Nov 2021, Richard Biener wrote:
>
> > >> +/* Unreachable code in if (0) block. */
> > >> +void baz(int *p)
> > >> +{
> > >> + if (0)
> > >> + {
> > >> + return; /* { dg-bogus "not reachable" } */
> > >
> > >Hmm? Why are you explicitely saying that warning here would be bogus?
> >
> > Because I don't think we want to warn here. Such code is common from
> > template instantiation or macro expansion.
>
> Like all code with an (const-propagated) explicit 'if (0)', which is of
> course the reason why -Wunreachable-code is a challenge.
OK, so I probably shouldn't have taken -Wunreachable-code but named
it somehow differently. We want to diagnose obvious programming
mistakes, not (source code) optimization opportunities. So
int foo (int i)
{
return i;
i += 1;
return i;
}
should be diagnosed for example but not so
int foo (int i)
{
if (USE_NOOP_FOO)
return i;
i += 1;
return i;
}
and compiling with -DUSE_NOOP_FOO=1
> IOW: I could
> accept your argument but then wonder why you want to warn about the second
> statement of the guarded block. The situation was:
>
> if (0) {
> return; // (1) don't warn here?
> whatever++; // (2) but warn here?
because as said above, the whatever++ will never be reachable even if
you change the condition in the if(). See my response to Martin where
I said I think if (0) of a block is a good way to comment it out
but keep it syntactically correct.
> }
>
> That seems even more confusing. So you don't want to warn about
> unreachable code (the 'return') but you do want to warn about unreachable
> code within unreachable code (point (2) is unreachable because of the
> if(0) and because of the return). If your worry is macro/template
> expansion resulting if(0)'s then I don't see why you would only disable
> warnings for some of the statements therein.
The point is not to disable the warning for some statements therein
but to avoid diagnosing following stmts.
> It seems we are actually interested in code unreachable via fallthrough or
> labels, not in all unreachable code, so maybe the warning is mis-named.
Yes, that's definitely the case - I was too lazy to re-use the old
option name here. But I don't have a good name at hand, maybe clang
has an option covering the cases I'm thinking about.
Btw, the diagnostic spotted qsort_chk doing
if (CMP (i1, i2))
break;
else if (CMP (i2, i1))
return ERR2 (i1, i2);
where ERR2 expands to a call to a noreturn void "returning"
qsort_chk_error, so the 'return' stmt is not reachable. Not exactly
a bug but somewhat difficult to avoid the diagnostic for. I suppose
the pointless 'return' is to make it more visible that the loop
terminates here (albeit we don't return normally).
Likewise we diagnose (c_tree_equal):
default:
gcc_unreachable ();
}
/* We can get here with --disable-checking. */
return false;
where the 'return false' is never reachable. The return was likely
inserted to avoid very strange error paths then the unreachable
falls through to some other random function.
> Btw. what does the code now do about this situation:
>
> if (0) {
> something++; // 1
> return; // 2
> somethingelse++; // 3
> }
>
> does it warn at (1) or not? (I assume it unconditionally warns at (3))
It warns at (3). It basically assumes that if (0) might become
if (1) in some other configuration and thus the diagnostic is
difficult to silence in source.
Any suggestion for a better option name?
Richard.
