Just realized the past two emails didn't go onto the mailing list, woops. On Wed, Jan 8, 2014 at 2:55 PM, [email protected] <[email protected]> wrote: > > > On Tue Jan 07 2014 at 7:44:54 PM, Michael Bao <[email protected]> wrote: >> >> > That sounds appropriate. That's what your original patch did, right? >> >> Correct. >> >> On Mon, Jan 6, 2014 at 4:32 PM, David Blaikie <[email protected]> wrote: >> > >> > >> > On Sun Jan 05 2014 at 2:36:58 PM, Michael Bao <[email protected]> >> > wrote: >> >> >> >> On Sat, Jan 4, 2014 at 1:39 PM, Aaron Ballman <[email protected]> >> >> 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). _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
