[clang-tools-extra] df5335a - [clang-tidy] readability-simplify-boolean-expr detects negated literals

Nathan James via cfe-commits cfe-commits at lists.llvm.org
Sat Aug 22 05:57:48 PDT 2020


Author: Nathan James
Date: 2020-08-22T13:57:36+01:00
New Revision: df5335a36d3d70927d1834f31c705e31b4b3701f

URL: https://github.com/llvm/llvm-project/commit/df5335a36d3d70927d1834f31c705e31b4b3701f
DIFF: https://github.com/llvm/llvm-project/commit/df5335a36d3d70927d1834f31c705e31b4b3701f.diff

LOG: [clang-tidy] readability-simplify-boolean-expr detects negated literals

Adds support for detecting cases like `if (!true) ...`.
Addresses [[ https://bugs.llvm.org/show_bug.cgi?id=47166 | readability-simplify-boolean-expr not detected for negated boolean literals. ]]

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D86176

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
    clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
    clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index 4c83499ff7ca..9dcb10b9d20c 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -58,16 +58,28 @@ const char SimplifyConditionDiagnostic[] =
 const char SimplifyConditionalReturnDiagnostic[] =
     "redundant boolean literal in conditional return statement";
 
-const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult &Result,
-                                         StringRef Id) {
-  const auto *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
-  return (Literal && Literal->getBeginLoc().isMacroID()) ? nullptr : Literal;
+const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
+                           StringRef Id) {
+  if (const Expr *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id))
+    return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
+  if (const auto *Negated = Result.Nodes.getNodeAs<UnaryOperator>(Id)) {
+    if (Negated->getOpcode() == UO_LNot &&
+        isa<CXXBoolLiteralExpr>(Negated->getSubExpr()))
+      return Negated->getBeginLoc().isMacroID() ? nullptr : Negated;
+  }
+  return nullptr;
+}
+
+internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
+  return expr(anyOf(cxxBoolLiteral(equals(Value)),
+                    unaryOperator(hasUnaryOperand(ignoringParenImpCasts(
+                                      cxxBoolLiteral(equals(!Value)))),
+                                  hasOperatorName("!"))));
 }
 
 internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
-  auto SimpleReturnsBool =
-      returnStmt(has(cxxBoolLiteral(equals(Value)).bind(Id)))
-          .bind("returns-bool");
+  auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
+                               .bind("returns-bool");
   return anyOf(SimpleReturnsBool,
                compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
@@ -269,16 +281,25 @@ std::string replacementExpression(const MatchFinder::MatchResult &Result,
   return asBool(getText(Result, *E), NeedsStaticCast);
 }
 
-const CXXBoolLiteralExpr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
   if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
     if (Bool->getValue() == !Negated)
       return Bool;
   }
+  if (const auto *Unary = dyn_cast<UnaryOperator>(Ret->getRetValue())) {
+    if (Unary->getOpcode() == UO_LNot) {
+      if (const auto *Bool =
+              dyn_cast<CXXBoolLiteralExpr>(Unary->getSubExpr())) {
+        if (Bool->getValue() == Negated)
+          return Bool;
+      }
+    }
+  }
 
   return nullptr;
 }
 
-const CXXBoolLiteralExpr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
+const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
   if (IfRet->getElse() != nullptr)
     return nullptr;
 
@@ -423,7 +444,7 @@ void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
                                                   StringRef BooleanId) {
   Finder->addMatcher(
       ifStmt(unless(isInTemplateInstantiation()),
-             hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
+             hasCondition(literalOrNegatedBool(Value).bind(BooleanId)))
           .bind(IfStmtId),
       this);
 }
@@ -433,8 +454,8 @@ void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
                                                   StringRef TernaryId) {
   Finder->addMatcher(
       conditionalOperator(unless(isInTemplateInstantiation()),
-                          hasTrueExpression(cxxBoolLiteral(equals(Value))),
-                          hasFalseExpression(cxxBoolLiteral(equals(!Value))))
+                          hasTrueExpression(literalOrNegatedBool(Value)),
+                          hasFalseExpression(literalOrNegatedBool(!Value)))
           .bind(TernaryId),
       this);
 }
@@ -465,12 +486,12 @@ void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
   auto SimpleThen =
       binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarAssign, MemAssign)),
                      hasLHS(expr().bind(IfAssignVariableId)),
-                     hasRHS(cxxBoolLiteral(equals(Value)).bind(IfAssignLocId)));
+                     hasRHS(literalOrNegatedBool(Value).bind(IfAssignLocId)));
   auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleThen)));
   auto SimpleElse =
       binaryOperator(hasOperatorName("="), hasLHS(anyOf(VarRef, MemRef)),
-                     hasRHS(cxxBoolLiteral(equals(!Value))));
+                     hasRHS(literalOrNegatedBool(!Value)));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
   if (ChainedConditionalAssignment)
@@ -495,7 +516,7 @@ void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
           hasAnySubstatement(
               ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
           hasAnySubstatement(returnStmt(has(ignoringParenImpCasts(
-                                            cxxBoolLiteral(equals(!Value)))))
+                                            literalOrNegatedBool(!Value))))
                                  .bind(CompoundReturnId)))
           .bind(Id),
       this);
@@ -529,10 +550,10 @@ void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   if (Result.Nodes.getNodeAs<TranslationUnitDecl>("top"))
     Visitor(this, Result).TraverseAST(*Result.Context);
-  else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
+  else if (const Expr *TrueConditionRemoved =
                getBoolLiteral(Result, ConditionThenStmtId))
     replaceWithThenStatement(Result, TrueConditionRemoved);
-  else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
+  else if (const Expr *FalseConditionRemoved =
                getBoolLiteral(Result, ConditionElseStmtId))
     replaceWithElseStatement(Result, FalseConditionRemoved);
   else if (const auto *Ternary =
@@ -574,8 +595,7 @@ void SimplifyBooleanExprCheck::issueDiag(
 }
 
 void SimplifyBooleanExprCheck::replaceWithThenStatement(
-    const MatchFinder::MatchResult &Result,
-    const CXXBoolLiteralExpr *TrueConditionRemoved) {
+    const MatchFinder::MatchResult &Result, const Expr *TrueConditionRemoved) {
   const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
   issueDiag(Result, TrueConditionRemoved->getBeginLoc(),
             SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
@@ -583,8 +603,7 @@ void SimplifyBooleanExprCheck::replaceWithThenStatement(
 }
 
 void SimplifyBooleanExprCheck::replaceWithElseStatement(
-    const MatchFinder::MatchResult &Result,
-    const CXXBoolLiteralExpr *FalseConditionRemoved) {
+    const MatchFinder::MatchResult &Result, const Expr *FalseConditionRemoved) {
   const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
   const Stmt *ElseStatement = IfStatement->getElse();
   issueDiag(Result, FalseConditionRemoved->getBeginLoc(),
@@ -631,7 +650,7 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
   for (++After; After != Compound->body_end() && *Current != Ret;
        ++Current, ++After) {
     if (const auto *If = dyn_cast<IfStmt>(*Current)) {
-      if (const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated)) {
+      if (const Expr *Lit = stmtReturnsBool(If, Negated)) {
         if (*After == Ret) {
           if (!ChainedConditionalReturn && BeforeIf)
             continue;

diff  --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
index 064953619f0b..af9017d4d3ae 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -51,11 +51,11 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
 
   void
   replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
-                           const CXXBoolLiteralExpr *BoolLiteral);
+                           const Expr *BoolLiteral);
 
   void
   replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult &Result,
-                           const CXXBoolLiteralExpr *FalseConditionRemoved);
+                           const Expr *FalseConditionRemoved);
 
   void
   replaceWithCondition(const ast_matchers::MatchFinder::MatchResult &Result,

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
index 3f49eec4c88a..a00b733ec1ea 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
@@ -98,6 +98,46 @@ void if_with_bool_literal_condition() {
   // CHECK-FIXES-NEXT: {{^  i = 11;$}}
 }
 
+void if_with_negated_bool_condition() {
+  int i = 10;
+  if (!true) {
+    i = 11;
+  } else {
+    i = 12;
+  }
+  i = 13;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:      {{^  int i = 10;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^    i = 12;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 13;$}}
+
+  i = 14;
+  if (!false) {
+    i = 15;
+  } else {
+    i = 16;
+  }
+  i = 17;
+  // CHECK-MESSAGES: :[[@LINE-6]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:      {{^  i = 14;$}}
+  // CHECK-FIXES-NEXT: {{^  {$}}
+  // CHECK-FIXES-NEXT: {{^    i = 15;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+  // CHECK-FIXES-NEXT: {{^  i = 17;$}}
+
+  i = 18;
+  if (!true) {
+    i = 19;
+  }
+  i = 20;
+  // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+  // CHECK-FIXES:      {{^  i = 18;$}}
+  // CHECK-FIXES-NEXT: {{^  $}}
+  // CHECK-FIXES-NEXT: {{^  i = 20;$}}
+}
+
 void operator_equals() {
   int i = 0;
   bool b1 = (i > 2);


        


More information about the cfe-commits mailing list