On Mon, May 23, 2016 at 05:53:38PM -0600, Martin Sebor wrote: > Sorry I'm a little late with my comments but I noticed one minor > problem (I raised bug 71249 for it since the patch has already > been checked in), and have a question about the hunk below: Firstly, thanks for looking into this.
> > @@ -1595,6 +1595,32 @@ gimplify_switch_expr (tree *expr_p, gimple_seq > > *pre_p) > > gimplify_ctxp->case_labels.create (8); > > > > gimplify_stmt (&SWITCH_BODY (switch_expr), &switch_body_seq); > > + > > + /* Possibly warn about unreachable statements between switch's > > + controlling expression and the first case. */ > > + if (warn_switch_unreachable > > + /* This warning doesn't play well with Fortran when optimizations > > + are on. */ > > + && !lang_GNU_Fortran () > > + && switch_body_seq != NULL) > > + { > > + gimple_seq seq = switch_body_seq; > > + if (gimple_code (switch_body_seq) == GIMPLE_BIND) > > + seq = gimple_bind_body (as_a <gbind *> (switch_body_seq)); > > + gimple *stmt = gimple_seq_first_stmt (seq); > > + enum gimple_code code = gimple_code (stmt); > > + if (code != GIMPLE_LABEL && code != GIMPLE_TRY) > > Why exempt GIMPLE_TRY? It suppresses the warning in cases like: > > switch (i) { > try { } catch (...) { } > case 1: ; > } > > (If excluding GIMPLE_TRY is unavoidable, it might be worthwhile > to add a comment to the code, and perhaps also mention it in > the documentation to preempt bug reports by nitpickers like me ;) I think I added that so that we do not warn on switch (i) { int A[3]; default: break; } because at the gimple level that looks like { int A[3]; try { <D.1751>: goto <D.1752>; } finally { A = {CLOBBER}; } } Another problem with try/finally is that it doesn't have a location so we'd jsut print useless cc1: warning: statement will never be executed [-Wswitch-unreachable] Though it seems so improbable that I don't really care about this case. > Finally, while even this simple warning can be useful, it would > be even more helpful if it could also point out other unreachable > statements within the body of the switch statements after > a break/goto/return and before a subsequent label. This could > be especially valuable with optimization to make possible > diagnosing non-trivial problems like this: > > switch (i) { > case 3: > if (i < 3) > return 1; > i = 8; > } > > (I realize this might be outside the scope of the feature request > and starting to creep into the -Wunreachable-code territory.) This really sounds like the old -Wunreachable stuff and I don't think it's limited to switches as this warning is. Nowadays we have stuff like gimple_stmt_may_fallthru so maybe that could be useful, but I'm not about to plunge into this mess anytime soon ;). Marek