[PATCH] D16259: Add clang-tidy readability-redundant-return check

[Aaron Ballman via cfe-commits]

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