[PATCH] NoReturn Warning: Removed Warning for Unreachable Return

[Michael Bao]

Just realized the past two emails didn't go onto the mailing list, woops.

On Wed, Jan 8, 2014 at 2:55 PM, dblaikie at gmail.com <dblaikie at gmail.com> wrote:
>
>
> On Tue Jan 07 2014 at 7:44:54 PM, Michael Bao <mike.h.bao at gmail.com> wrote:
>>
>> > That sounds appropriate. That's what your original patch did, right?
>>
>> Correct.
>>
>> On Mon, Jan 6, 2014 at 4:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Sun Jan 05 2014 at 2:36:58 PM, Michael Bao <mike.h.bao at gmail.com>
>> > wrote:
>> >>
>> >> On Sat, Jan 4, 2014 at 1:39 PM, Aaron Ballman <aaron at aaronballman.com>
>> >> wrote:
>> >> >
>> >> > That makes sense as to why you'd move it, but I'm still concerned
>> >> > about this changing the semantics. For instance, this can now fire
>> >> > for
>> >> > ObjCMethodDecl objects, where it used to not be possible. This could
>> >> > be a bug fix, but it would also require further testing (and
>> >> > confirmation from some of the ObjC experts as to whether this is
>> >> > desirable).
>> >>
>> >> Hm, wouldn't the wrap around the diagnostic call with
>> >>
>> >> if (const FunctionDecl *FD = getCurFunctionDecl()) {
>> >> }
>> >>
>> >> prevent it from firing for ObjCMethodDecl objects?
>> >>
>> >> But looking at the bigger picture, it seems like Joerg was more so
>> >> envisioning that the warning be replaced with a better warning in the
>> >> case that the 'return' statement is unreachable. However, the default
>> >> behavior for the unreachable code warning is to ignore it unless
>> >> -Wunreachable-code is passed into the compiler.
>> >>
>> >> So in the case that the 'return' statement is reachable, we definitely
>> >> want to emit the 'warn_noreturn_function_has_return_expr' warning.
>> >
>> >
>> > Right - this should be a runtime diagnostic (ie: only fire on reachable
>> > code). Return is reachable so warn. This should equally fire for falling
>> > off
>> > the end of the function, of course. ( in the absence of an explicit
>> > return)
>> >
>> >>
>> >> However, when it isn't, I think it would be best to only emit a
>> >> warning only if -Wunreachable-code is passed into the compiler. Does
>> >> that sound reasonable?
>> >
>> >
>> > Sounds like the right thing to me. -Wunreachable-code still needs a lot
>> > of
>> > smarts/improvements to reduce the false-positive rate & I wouldn't want
>> > to
>> > duplicate that effort or lower the diagnostic quality in this case.
>> >
>> > (the only 'out' here is if a noreturn function gives us a higher/better
>> > signal for bug finding
>>
>> The only thing I can potentially think of is if someone puts in an
>> unreachable return statement in a noreturn function without realizing
>> that the function has the noreturn attribute.
>
>
> I think the point is that we should just catch that with -Wunreachable-code
> - but maybe that's just my perspective & perhaps the other participants have
> other ideas (I've not entirely followed this thread, sorry).
>
> (granted -Wunreachable-code is... bad at the moment so perhaps some stopgap
> higher-quality warnings could be useful - but even in the case you're
> describing I'm concerned that someone might have a collection of macros that
> under one condition lead to a noreturn function containing an unreachable
> return, and another condition lead to the function not being marked noreturn
> and the return being reachable - but perhaps that's a stretch not worth
> worrying about until we have some proof)
>

I agree with you in this regard.

>>
>>
>> In this case, would my patch be a valid way of going about doing this?
>> Aaron had concerns about moving the Diag warning 200 lines down;
>> however, I don't really see any way of doing the runtime diagnosis
>> otherwise (granted I am still not very familiar with the Clang
>> codebase yet).