aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.
I would like to see some additional tests to ensure that this does not turn
dead code into live code with the fix. e.g.,
void g();
void f() {
return;
g(); // This would become live code if return were removed.
}
A similar test for continue would be appreciated as well. Tests for range-based
for loops also.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:23
@@ +22,3 @@
+ Finder->addMatcher(
+ functionDecl(isDefinition(), returns(asString("void")),
+ has(compoundStmt(hasAnySubstatement(returnStmt()))))
----------------
Would be better written as: `returns(voidType())` instead of using `asString()`.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:24
@@ +23,3 @@
+ functionDecl(isDefinition(), returns(asString("void")),
+ has(compoundStmt(hasAnySubstatement(returnStmt()))))
+ .bind("fn"),
----------------
Would be best to restrict this to a return statement that has no expression if
we don't want to diagnose this:
```
void g();
void f() {
return g();
}
```
Either way, it would be good to have a test that ensures this isn't mangled.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:28
@@ +27,3 @@
+ auto CompoundContinue =
has(compoundStmt(hasAnySubstatement(continueStmt())));
+ Finder->addMatcher(forStmt(CompoundContinue).bind("for"), this);
+ Finder->addMatcher(whileStmt(CompoundContinue).bind("while"), this);
----------------
I think you also want to match cxxForRangeStmt() in the same way.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:34
@@ +33,3 @@
+void RedundantReturnCheck::check(const MatchFinder::MatchResult &Result) {
+ if (const auto *Fn = Result.Nodes.getNodeAs<FunctionDecl>("fn")) {
+ checkRedundantReturn(Result, Fn);
----------------
Elide braces for these if-else statements.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:75
@@ +74,3 @@
+void RedundantReturnCheck::checkRedundantContinue(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const CompoundStmt *Block) {
----------------
No need for `ast_matchers`.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.cpp:77
@@ +76,3 @@
+ const CompoundStmt *Block) {
+ if (Block) {
+ CompoundStmt::const_reverse_body_iterator last = Block->body_rbegin();
----------------
Please consolidate this duplicated code.
================
Comment at: clang-tidy/readability/RedundantReturnCheck.h:26
@@ +25,3 @@
+///
http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-return.html
+class RedundantReturnCheck : public ClangTidyCheck {
+public:
----------------
Since this also handling continue, I think this would be
SpuriousFlowControlCheck instead?
http://reviews.llvm.org/D16259
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
