Re: [PATCH] Remove more stray returns and gcc_unreachable ()s

[Richard Biener <rguenther@suse.de>]

On Mon, 29 Nov 2021, Martin Sebor wrote:

> On 11/29/21 11:53 AM, Martin Sebor wrote:
> > On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote:
> >> This removes more cases that appear when bootstrap with
> >> -Wunreachable-code-return progresses.
> >>
> > ...
> >> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
> >> index 8ee0529d5a8..18e03c4cb96 100644
> >> --- a/gcc/sel-sched-ir.h
> >> +++ b/gcc/sel-sched-ir.h
> >> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb)
> >>       default:
> >>         return bb->next_bb;
> >>       }
> >> -
> >> -  gcc_unreachable ();
> >>   }
> > 
> > Just skiming the changes out of curiosity, this one makes me
> > wonder if the warning shouldn't be taught to avoid triggering
> > on calls to __builtin_unreachable().  They can help make code
> > more readable (e.g., after a case and switch statement that
> > handles all values).
> 
> I see someone else raised the same question in a patch I hadn't
> gotten to yet:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html
> 
> If you do end up removing the gcc_unreachable() calls, I would
> suggest to replace them with a comment so as not to lose
> the readability benefit.
> 
> But I still wonder if it might make sense to teach the warning
> not just about __builtin_unreachable() but also about noreturn
> calls like abort() that (as you explained in the thread above)
> gcc_unreachable() expands to.  Is there a benefit to warning
> on such calls?

I'm not sure.  I've chosen to eliminate only the "obvious"
cases, like above where there's a default: that returns immediately
visible (not always in the patch context).  I've left some in
the code base where that's not so obvious.

IMHO making the flow obvious without a unreachable marker is
superior to obfuscating it and clearing that up with one.

Yes, I thought about not diagnosing things like

   return 1;
   return 1;

but then what about

   return 1;
   return 0;

?  I've seen cases like

   gcc_unreachable ();
   return 0;

was that meant to be

   return 0;
   gcc_unreachable ();

?  So it's not entirely clear.  I think that if there was a way
to denote definitive 'code should not reach here' function
(a new attribute?) then it would make sense to not warn about
control flow not reaching that.  But then it would make sense
to warn about stmts following such annotation.

Richard.