Page MenuHomePhabricator

[clang-tidy] enhance readability-else-after-return
ClosedPublic

Authored by omtcyfz on Aug 8 2016, 7:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyfz updated this revision to Diff 67160.Aug 8 2016, 7:26 AM
omtcyfz retitled this revision from to [clang-tidy] enhance readability-else-after-return.
omtcyfz updated this object.
omtcyfz added a reviewer: alexfh.
omtcyfz added a subscriber: cfe-commits.
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
38–40 ↗(On Diff #67160)

I think it might be better to specifically identify the flow control construct used instead of a laundry list of possibilities. Then it can be "do not use '%select{else|else if}0' after '%1'".

alexfh requested changes to this revision.Aug 8 2016, 8:25 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
38–40 ↗(On Diff #67160)

The wording is actually good. For documentation (currently, http://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html is not extremely verbose ;)).

Otherwise, I agree with Aaron: the warning should be precise to avoid confusion.

Also, no need for a separate variable to keep the message, IMO.

This revision now requires changes to proceed.Aug 8 2016, 8:25 AM
omtcyfz updated this revision to Diff 67347.Aug 9 2016, 7:41 AM
omtcyfz edited edge metadata.
omtcyfz marked 2 inline comments as done.

Address the comments.

alexfh requested changes to this revision.Aug 9 2016, 8:10 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/ElseAfterReturnCheck.cpp
25 ↗(On Diff #67347)

Let's structure this differently to reduce code duplication:

auto returnLikeStmt = stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"), ...));
Finder->addMatcher(stmt(forEach(ifStmt(hasThen(anyOf(returnLikeStmt, compoundStmt(has(returnLikeStmt)))), hasElse(...
This revision now requires changes to proceed.Aug 9 2016, 8:10 AM
alexfh added inline comments.Aug 9 2016, 8:18 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

This won't work in MSVC2013, I think. Just add a const char *Labels[] = {"return", ... (add more consts, constexprs or statics, if you like ;)

51 ↗(On Diff #67347)

Please use diagnostic formatting facilities instead of string concatenation:

diag(L, "xxx '%0' yyy") << ControlFlowInterruptor;
docs/clang-tidy/checks/readability-else-after-return.rst
10 ↗(On Diff #67347)

I would omit goto, since it's unclear, where the target label is (and it's not a common construct anyway).

test/clang-tidy/readability-else-after-return.cpp
33 ↗(On Diff #67347)

CHECK-FIXES pattern should be sticter. Current pattern will match for any // comment occurrence anywhere in the test file, even if it's on a different line and still prefixed with else.

Two action items here: 1. make comments and patterns unique, 2. anchor patterns to the start of line ({{ˆ}}).

omtcyfz added inline comments.Aug 9 2016, 10:38 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

Hm, I'm confused. What exactly wouldn't work in MSVC?

test/clang-tidy/readability-else-after-return.cpp
33 ↗(On Diff #67347)

Why make comments and patterns unique? If I anchor patterns to the start of line, it will only look at the line above, why does it need to be unique then?

aaron.ballman added inline comments.Aug 9 2016, 10:43 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

This one *will* work in MSVC 2013 (it was a different initializer list that @alexfh was thinking of, perhaps, where the deduced type of the list was a bit more complex). I just tried it out and the above construct compiles fine with MSVC.

That being said, const auto & is the wrong type here, it should be const auto *.

omtcyfz added inline comments.Aug 9 2016, 10:47 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

Hm, I'm confused. Why should it be a pointer?

So, when I have something like:

std::vector<unsigned> vec = {1, 2, 3};
for (const auto &x : vec) ...

This is right, I guess. Why is that different with init list?

aaron.ballman added inline comments.Aug 9 2016, 11:01 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

The deduced type for the code as-written is const char * const &, which isn't wrong, it's just not particularly useful. If you switch to const auto *, the deduced type is the much more reasonable const char *.

omtcyfz added inline comments.Aug 9 2016, 11:05 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

Ah... Thank you very much! Now I get it. I thought for whatever reason that there is implicit conversion to std::string.

alexfh added inline comments.Aug 9 2016, 11:59 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
45 ↗(On Diff #67347)

This one *will* work in MSVC 2013

With MSVC you never know what will work, and what won't... Thanks for verifying!

alexfh added inline comments.Aug 9 2016, 12:09 PM
test/clang-tidy/readability-else-after-return.cpp
33 ↗(On Diff #67347)

Why make comments and patterns unique?

Because FileCheck is way simpler than one usually thinks of it.

It doesn't maintain any relation between the location of the check line and the input line. So your assumption that

it will only look at the line above

is wrong.

The only thing FileCheck cares about is that the input contains a subsequence of lines that match all the CHECK patterns (a bit more complicated, if CHECK-NOT, CHECK-NEXT, CHECK-SAME, CHECK-DAG, or CHECK-LABEL are in use, but the basic idea is roughly the same).

So, if all CHECK-FIXES patterns in this test are // comment, then any input that contains at least the same number of // comment substrings on different lines will pass the test.

Please read http://llvm.org/docs/CommandGuide/FileCheck.html as well.

omtcyfz updated this revision to Diff 67475.Aug 10 2016, 12:58 AM
omtcyfz edited edge metadata.
omtcyfz marked 7 inline comments as done.

Address comments.

omtcyfz updated this revision to Diff 67476.Aug 10 2016, 1:00 AM
omtcyfz edited edge metadata.

Make helper matcher const.

aaron.ballman added inline comments.Aug 10 2016, 5:11 AM
docs/clang-tidy/checks/readability-else-after-return.rst
8 ↗(On Diff #67476)

enforcements (plural)

10 ↗(On Diff #67476)

I would remove the "etc", since this check does not handle such cases.

12 ↗(On Diff #67476)

Following -> The following
check would work -> the check works

14 ↗(On Diff #67476)

Since we're talking about LLVM coding conventions, can you format this like LLVM code (elide braces, etc)?

14 ↗(On Diff #67476)

This is not the proper way to introduce a code block. Should use .. code-block:: c++ and then indent the code blocks.

alexfh added inline comments.Aug 10 2016, 5:23 AM
clang-tidy/readability/ElseAfterReturnCheck.cpp
40 ↗(On Diff #67476)

No braces around single-line if bodies, please. Clang-tidy code is mostly consistent wrt this.

docs/clang-tidy/checks/readability-else-after-return.rst
14 ↗(On Diff #67476)

I think, the braces are fine, since the example needs to demonstrate that the check is able to remove them.

aaron.ballman added inline comments.Aug 10 2016, 5:26 AM
docs/clang-tidy/checks/readability-else-after-return.rst
14 ↗(On Diff #67476)

Okay, that's a fair, but subtle, reason to leave them in.

omtcyfz updated this revision to Diff 67512.Aug 10 2016, 5:27 AM
omtcyfz marked 4 inline comments as done.

Address comments.

aaron.ballman accepted this revision.Aug 10 2016, 5:33 AM
aaron.ballman edited edge metadata.

LGTM!

omtcyfz updated this revision to Diff 67513.Aug 10 2016, 5:35 AM
omtcyfz edited edge metadata.

Remove braces around single statement in for and if as Alex proposed.

alexfh accepted this revision.Aug 10 2016, 10:22 AM
alexfh edited edge metadata.

LG

This revision is now accepted and ready to land.Aug 10 2016, 10:22 AM
This revision was automatically updated to reflect the committed changes.