Page MenuHomePhabricator

clang-tidy: Add initial check for "Don't use else after return".
ClosedPublic

Authored by djasper on Jan 12 2015, 9:01 AM.

Details

Reviewers
alexfh
Summary

As per the LLVM coding standards:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Initial version, probably still quite a bit to do until this is really useful.

Diff Detail

Event Timeline

djasper updated this revision to Diff 18020.Jan 12 2015, 9:01 AM
djasper retitled this revision from to clang-tidy: Add initial check for "Don't use else after return"..
djasper updated this object.
djasper edited the test plan for this revision. (Show Details)
djasper added a reviewer: alexfh.
djasper added a subscriber: Unknown Object (MLST).

Not your problem, but I'm wondering: If/when/how we'll be able to integrate clang-tidy checks into the compile step for developers. This warning and many others in clang-tidy ought to be cheap enough to run at compile time and as hard errors just like many real clang warnings (the only reason they're not is that they're stylistic in nature and so don't meet that bar for the compiler warnings - not because they aren't cheap, low false positive, etc). It'd be nice not to have these as asynchronous results but as errors during the build.

clang-tidy/misc/ElseAfterReturnCheck.cpp
38 ↗(On Diff #18020)

Is this the "quite a bit to do" you are referring to? Or are there other holes you know need fixing? (perhaps some could be included in the test case to give an idea of why this isn't ready for prime-time yet)

silvas added a subscriber: silvas.Jan 12 2015, 3:46 PM

Not your problem, but I'm wondering: If/when/how we'll be able to integrate clang-tidy checks into the compile step for developers. This warning and many others in clang-tidy ought to be cheap enough to run at compile time and as hard errors just like many real clang warnings (the only reason they're not is that they're stylistic in nature and so don't meet that bar for the compiler warnings - not because they aren't cheap, low false positive, etc). It'd be nice not to have these as asynchronous results but as errors during the build.

Maybe there's scope for a way to add warnings/errors which are run as a separate AST consumer, rather than integrated into the parsing/lexing code path. That way, you don't pay for them if you don't use them (also you don't pay for them in added complexity within the parsing/lexing code). I think most AST-based clang-tidy checks would fit that model. We also have a warning we added internally that would be very nice to cleanly segregate out of the main code path like that.

alexfh edited edge metadata.Jan 13 2015, 8:20 AM

The rule applies to other control statements as well, such as break, continue and throw (may be also goto, but who cares about this rule when a goto is used?). Can we handle them in the same check? Then the name may need to be changed.

clang-tidy/misc/ElseAfterReturnCheck.cpp
34 ↗(On Diff #18020)

clang-format?

35 ↗(On Diff #18020)

Maybe explain what "here" means? How about "don't use else after a return"?

clang-tidy/misc/MiscTidyModule.cpp
32 ↗(On Diff #18020)

It's definitely a readability check, and it's style-specific. Thus, I think, it's better to put this check in the readability/ directory and then include in the styles it's relevant to (currently, llvm, maybe also google, if a similar rule is added there).

test/clang-tidy/misc-else-after-return.cpp
15 ↗(On Diff #18020)

Please add a line number to avoid incorrect matches.

It does apply to other statements and we are going to implement those in this check, but I'd like the check to have the same name as the rule in the coding standard. Also, after "return" seems to be by far the most frequent case.

clang-tidy/misc/ElseAfterReturnCheck.cpp
34 ↗(On Diff #18020)

Done.

35 ↗(On Diff #18020)

Done.

38 ↗(On Diff #18020)

That's part of the analysis. Happy to do all that, but don't have time at the moment. I think the check is useful as is, though.

test/clang-tidy/misc-else-after-return.cpp
15 ↗(On Diff #18020)

Done.

djasper updated this revision to Diff 18166.Jan 14 2015, 10:49 AM
djasper edited edge metadata.
alexfh accepted this revision.Jan 14 2015, 11:28 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
22

Please add a FIXME to support continue, break and throw.

35

Looks like a debug code. Is it needed?

40

const auto *CS?

41

Please add a FIXME to come up with a good way to ensure that the formatting is updated for the whole compound statement. I guess that the "format changed lines" strategy will fail here. Maybe we'll need to remove some leading whitespace from each line to ensure it's treated as changed.

This revision is now accepted and ready to land.Jan 14 2015, 11:28 AM
djasper added inline comments.Jan 14 2015, 11:36 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
22

Done.

35

Done.

40

Done.

41

If it doesn't work, I actually want to fix clang-format to handle this correctly. Might come up in other workflows, too and we want it to work anywhere (git clang-format, etc.).

djasper closed this revision.Jan 14 2015, 11:39 AM

Submitted as r226025.