Page MenuHomePhabricator

Simplify CheckFallThroughForBody
Needs ReviewPublic

Authored by gromer on Sep 7 2018, 1:47 PM.

Details

Reviewers
rsmith
Summary

Split CheckFallThroughForBody into separate implementations for blocks, lambdas, coroutines, and all other functions. This simplifies the code because virtually every part of it varies depending on what kind of function is being processed; it's both clearer and safer to branch once at the beginning, rather than branch repeatedly (sometimes on subtly different conditions) and/or rely on layers of indirection. Notice, for example, how this refactoring surfaces the inconsistency between the fast and slow paths for coroutines, and enables the fast-path condition for functions to take advantage of more information about what kinds of diagnostics the slow path can produce (see specifically how SuggestNoReturn now feeds into the early-return condition).

Diff Detail

Event Timeline

gromer created this revision.Sep 7 2018, 1:47 PM
rsmith added a comment.Sep 7 2018, 2:50 PM

I'm not a fan of the duplication introduced here, but the new code is definitely more obvious. On balance, this seems like a small improvement, so let's go for it.

b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
542–544 ↗(On Diff #164502)

Hmm. It really doesn't make any sense to apply cpu_dispatch to a lambda, because the call operator can't be overloaded. I don't think this special case warning suppression is worthwhile. (Rather, we should probably disallow the attribute on lambdas; I've asked Erich Keane about that.)

595 ↗(On Diff #164502)

I know this is just rearranged from where it was before, but... can you remove the redundant parens here, and run the patch through clang-format?

601 ↗(On Diff #164502)

This if and the one below are redundant now.

gromer updated this revision to Diff 166521.Sep 21 2018, 11:21 AM
gromer marked 2 inline comments as done.
gromer added inline comments.Sep 21 2018, 11:31 AM
b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
601 ↗(On Diff #164502)

Leaving aside the mismatch between the locations passed to isIgnored and Diag, I believe this if still has an observable effect (namely suppressing the diagnostic) in the case where ReturnsValue is false, HasNoReturnAttr is true, and neither diagnostic is ignored.

Even if I'm wrong about that, it's not clear to me whether the error is here or in the early return condition (as noted in the FIXME above, the two do not appear to match up).

gromer updated this revision to Diff 166545.Sep 21 2018, 1:43 PM
gromer updated this revision to Diff 166558.Sep 21 2018, 2:00 PM

OK, the diffs are now un-borked. Sorry for the flailing incompetence.