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

[njames93]

Summary

Adds support for detecting cases like if (!true) ....
Addresses readability-simplify-boolean-expr not detected for negated boolean literals.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

66

I'd like to see this handled recursively instead so that we properly handle more esoteric logic (but it still shows up from time to time) like !!foo: https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21&search=Search

This is a case where I don't think the simplification should happen -- e.g., the code is usually subtly incorrect when converted to remove the !!. It's less clear to me whether the same is true for something like !!! being converted to !, but that's not a case I'm really worried about either.

74

Oof, but handling it here may be tricky...

289

Same here.

[njames93]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

66

As this is only looking for boolean literals that shouldn't matter
!!true is identical to true.
I get putting !! in front of expressions that aren't boolean has an effect, but we aren't looking for that in this case.

74

A custom matcher could be made for this that could traverse a UnaryOperator

289

This could be easily handled with some recursion.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

66

Oh, derp, I saw "SimplifyBooleanExprCheck" and assumed it was simplifying arbitrary boolean expressions, not just literals. I am now far less worried about supporting this case, it's up to you if you want to do the extra work. Thank you for clarifying and sorry for my think-o!

[njames93]

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

66

Personally I don't think the extra complexity is worth it. Usages of !true, I expect are rather low but likely show up in a few places, however I doubt anyone would use !!true.

[aaron.ballman]

LGTM, thank you for the fix!

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

66

Agreed.

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h

private:
   void matchIfAssignsBool(ast_matchers::MatchFinder *Finder, bool Value,
                           StringRef Id);
 
   void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
                                   StringRef Id);
 
   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,
                        const ConditionalOperator *Ternary,
                        bool Negated = false);
 
   void replaceWithReturnCondition(
       const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

 
 const char SimplifyOperatorDiagnostic[] =
     "redundant boolean literal supplied to boolean operator";
 const char SimplifyConditionDiagnostic[] =
     "redundant boolean literal in if statement condition";
 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,
+  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)))
+  auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
                                .bind("returns-bool");
   return anyOf(SimpleReturnsBool,
                compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
 
 bool needsParensAfterUnaryNegation(const Expr *E) {
   E = E->IgnoreImpCasts();
   if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
     return true;
if (needsNullPtrComparison(E))
     return compareExpressionToNullPtr(Result, E, true);
 
   if (needsZeroComparison(E))
     return compareExpressionToZero(Result, E, true);
 
   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;
 
   if (const auto *Ret = dyn_cast<ReturnStmt>(IfRet->getThen()))
     return stmtReturnsBool(Ret, Negated);
 
   if (const auto *Compound = dyn_cast<CompoundStmt>(IfRet->getThen())) {
     if (Compound->size() == 1) {
void SimplifyBooleanExprCheck::reportBinOp(
   }
 }
 
 void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef BooleanId) {
   Finder->addMatcher(
       ifStmt(unless(isInTemplateInstantiation()),
-             hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
+             hasCondition(literalOrNegatedBool(Value).bind(BooleanId)))
           .bind(IfStmtId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef TernaryId) {
   Finder->addMatcher(
       conditionalOperator(unless(isInTemplateInstantiation()),
-                          hasTrueExpression(cxxBoolLiteral(equals(Value))),
-                          hasFalseExpression(cxxBoolLiteral(equals(!Value))))
+                          hasTrueExpression(literalOrNegatedBool(Value)),
+                          hasFalseExpression(literalOrNegatedBool(!Value)))
           .bind(TernaryId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   if (ChainedConditionalReturn)
     Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   auto VarAssign = declRefExpr(hasDeclaration(decl().bind(IfAssignVarId)));
   auto VarRef = declRefExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
   auto MemAssign = memberExpr(hasDeclaration(decl().bind(IfAssignVarId)));
   auto MemRef = memberExpr(hasDeclaration(equalsBoundNode(IfAssignVarId)));
   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)
     Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
                               hasThen(Then), hasElse(Else))
                            .bind(Id),
                        this);
   else
     Finder->addMatcher(ifStmt(unless(isInTemplateInstantiation()),
                               unless(hasParent(ifStmt())), hasThen(Then),
                               hasElse(Else))
                            .bind(Id),
                        this);
 }
 
 void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
                                                           bool Value,
                                                           StringRef Id) {
   Finder->addMatcher(
       compoundStmt(
           unless(isInTemplateInstantiation()),
           hasAnySubstatement(
               ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
           hasAnySubstatement(returnStmt(has(ignoringParenImpCasts(
-                                            cxxBoolLiteral(equals(!Value)))))
+                                            literalOrNegatedBool(!Value))))
                                  .bind(CompoundReturnId)))
           .bind(Id),
       this);
 }
 
 void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
   Options.store(Opts, "ChainedConditionalAssignment",
void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
 
   matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
   matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
 }
 
 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 =
                Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
     replaceWithCondition(Result, Ternary);
   else if (const auto *TernaryNegated =
                Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
     replaceWithCondition(Result, TernaryNegated, true);
CharSourceRange CharRange =
                                *Result.SourceManager, getLangOpts());
 
   DiagnosticBuilder Diag = diag(Loc, Description);
   if (!containsDiscardedTokens(Result, CharRange))
     Diag << FixItHint::CreateReplacement(CharRange, Replacement);
 }
 
 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(),
             getText(Result, *IfStatement->getThen()));
 }
 
 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(),
             SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
             ElseStatement ? getText(Result, *ElseStatement) : "");
 }
 
 void SimplifyBooleanExprCheck::replaceWithCondition(
void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
   //    `false`.
   assert(Compound->size() >= 2);
   const IfStmt *BeforeIf = nullptr;
   CompoundStmt::const_body_iterator Current = Compound->body_begin();
   CompoundStmt::const_body_iterator After = Compound->body_begin();
   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;
 
           const Expr *Condition = If->getCond();
           std::string Replacement =
               "return " + replacementExpression(Result, Negated, Condition);
           issueDiag(

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

void if_with_bool_literal_condition() {
   }
   i = 11;
   // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
   // CHECK-FIXES:      {{^  i = 8;$}}
   // CHECK-FIXES-NEXT: {{^  $}}
   // 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);
   if (b1 == true) {
     // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: {{.*}} to boolean operator
     // CHECK-FIXES: {{^  if \(b1\) {$}}
     i = 5;
   } else {