[clang-tidy] Enhance clang-tidy readability-simplify-boolean-expr check...

Enhance clang-tidy readability-simplify-boolean-expr check to handle chained
conditional assignment and chained conditional return.

Based on feedback from applying this tool to the clang/LLVM codebase, this
changeset improves the readability-simplify-boolean-expr check so that
conditional assignment or return statements at the end of a chain of if/else if
statements are left unchanged unless a configuration option is supplied.

http://reviews.llvm.org/D8996

Patch by Richard Thomson!



git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@237541 91177308-0d34-0410-b5e6-96231b3b80d8

5 files changed, 175 insertions, 16 deletions

diff --git a/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index c9a1182a..f7754e44 100644
--- a/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -108,20 +108,25 @@ std::string replacementExpression(const MatchFinder::MatchResult &Result,
       StringRef NegatedOperator = negatedOperator(BinOp);
       if (!NegatedOperator.empty()) {
         return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
-                " " + getText(Result, *BinOp->getRHS()))
-            .str();
+                " " + getText(Result, *BinOp->getRHS())).str();
       }
     }
   }
   StringRef Text = getText(Result, *E);
   return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
                                                       : "!" + Text)
-                  : Text)
-      .str();
+                  : Text).str();
 }
 
 } // namespace
 
+SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)),
+      ChainedConditionalAssignment(
+          Options.get("ChainedConditionalAssignment", 0U)) {}
+
 void SimplifyBooleanExprCheck::matchBoolBinOpExpr(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef OperatorName,
@@ -199,10 +204,18 @@ void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
-  Finder->addMatcher(ifStmt(isExpansionInMainFile(),
-                            hasThen(ReturnsBool(Value, ThenLiteralId)),
-                            hasElse(ReturnsBool(!Value))).bind(Id),
-                     this);
+  if (ChainedConditionalReturn) {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              hasThen(ReturnsBool(Value, ThenLiteralId)),
+                              hasElse(ReturnsBool(!Value))).bind(Id),
+                       this);
+  } else {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())),
+                              hasThen(ReturnsBool(Value, ThenLiteralId)),
+                              hasElse(ReturnsBool(!Value))).bind(Id),
+                       this);
+  }
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
@@ -220,9 +233,22 @@ void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
       hasRHS(boolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
-  Finder->addMatcher(
-      ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
-      this);
+  if (ChainedConditionalAssignment) {
+    Finder->addMatcher(
+        ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
+        this);
+  } else {
+    Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                              unless(hasParent(ifStmt())), hasThen(Then),
+                              hasElse(Else)).bind(Id),
+                       this);
+  }
+}
+
+void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
+  Options.store(Opts, "ChainedConditionalAssignment",
+                ChainedConditionalAssignment);
 }
 
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
diff --git a/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tidy/readability/SimplifyBooleanExprCheck.h
index 5bb67a47..365d8211 100644
--- a/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ b/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -30,15 +30,25 @@ namespace readability {
 /// `e ? false : true`                         becomes `!e`
 /// `if (true) t(); else f();`                 becomes `t();`
 /// `if (false) t(); else f();`                becomes `f();`
-/// `if (e) return true; else return false;`   becomes `return (e);`
-/// `if (e) return false; else return true;`   becomes `return !(e);`
+/// `if (e) return true; else return false;`   becomes `return e;`
+/// `if (e) return false; else return true;`   becomes `return !e;`
 /// `if (e) b = true; else b = false;`         becomes `b = e;`
-/// `if (e) b = false; else b = true;`         becomes `b = !(e);`
+/// `if (e) b = false; else b = true;`         becomes `b = !e;`
+///
+/// Parenthesis from the resulting expression `e` are removed whenever
+/// possible.
+///
+/// When a conditional boolean return or assignment appears at the end of a
+/// chain of `if`, `else if` statements, the conditional statement is left
+/// unchanged unless the option `ChainedConditionalReturn` or
+/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
+/// The default value for both options is zero.
 ///
 class SimplifyBooleanExprCheck : public ClangTidyCheck {
 public:
-  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
@@ -92,6 +102,9 @@ private:
   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
                         const IfStmt *If, bool Negated = false);
+
+  const bool ChainedConditionalReturn;
+  const bool ChainedConditionalAssignment;
 };
 
 } // namespace readability
diff --git a/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp b/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp
new file mode 100644
index 00000000..a4e60263
--- /dev/null
+++ b/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp
@@ -0,0 +1,35 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: 1}]}" --
+// REQUIRES: shell
+
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  if (i < 0) {
+    b = true;
+  } else if (i < 10) {
+    b = false;
+  } else if (i > 20) {
+    b = true;
+  } else {
+    b = false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    b = false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else b = i > 20;{{$}}
+}
+
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+  // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: {{.*}} in conditional assignment
+  // CHECK-FIXES:      {{^}}  else if (i < 10)
+  // CHECK-FIXES-NEXT: {{^}}    b = false;
+  // CHECK-FIXES-NEXT: {{^}}  else b = i > 20;
+}
diff --git a/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp b/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
new file mode 100644
index 00000000..d50c13ac
--- /dev/null
+++ b/test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
@@ -0,0 +1,33 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-simplify-boolean-expr %t -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: 1}]}" --
+// REQUIRES: shell
+
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]
+  // CHECK-FIXES:      {{^}}  } else if (i < 10) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    return false;{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  } else return i > 20;{{$}}
+}
+
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  else if (i > 20)
+    return true;
+  else
+    return false;
+  // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+  // CHECK-FIXES:      {{^}}  else if (i < 10)
+  // CHECK-FIXES-NEXT: {{^}}    return false;
+  // CHECK-FIXES-NEXT: {{^}}  else return i > 20;
+}
diff --git a/test/clang-tidy/readability-simplify-bool-expr.cpp b/test/clang-tidy/readability-simplify-bool-expr.cpp
index 0658a17b..708270fe 100644
--- a/test/clang-tidy/readability-simplify-bool-expr.cpp
+++ b/test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -541,3 +541,55 @@ void complex_conditional_assignment_statements(int i) {
   } else
     f = false;
 }
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_compound_return(int i) {
+  if (i < 0) {
+    return true;
+  } else if (i < 10) {
+    return false;
+  } else if (i > 20) {
+    return true;
+  } else {
+    return false;
+  }
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_conditional_return(int i) {
+  if (i < 0)
+    return true;
+  else if (i < 10)
+    return false;
+  else if (i > 20)
+    return true;
+  else
+    return false;
+}
+
+// unchanged: chained assignments, but ChainedConditionalAssignment not set
+void chained_conditional_compound_assignment(int i) {
+  bool b;
+  if (i < 0) {
+    b = true;
+  } else if (i < 10) {
+    b = false;
+  } else if (i > 20) {
+    b = true;
+  } else {
+    b = false;
+  }
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+void chained_conditional_assignment(int i) {
+  bool b;
+  if (i < 0)
+    b = true;
+  else if (i < 10)
+    b = false;
+  else if (i > 20)
+    b = true;
+  else
+    b = false;
+}