[clang-tidy] avoid false postive when ignore macro

Fixes: #91487
llvm · May 10, 2024 · e23675e · e23675e
1 parent 023cdfc
commit e23675e
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 29 deletions.
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "SimplifyBooleanExprCheck.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -280,9 +281,8 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
     if (!S) {
       return true;
     }
-    if (Check->IgnoreMacros && S->getBeginLoc().isMacroID()) {
+    if (Check->canBeBypassed(S))
       return false;
-    }
     if (!shouldIgnore(S))
       StmtStack.push_back(S);
     return true;
@@ -513,17 +513,25 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
     return true;
   }
 
-  static bool isUnaryLNot(const Expr *E) {
-    return isa<UnaryOperator>(E) &&
+  static bool isExpectedUnaryLNot(SimplifyBooleanExprCheck *Check,
+                                  const Expr *E) {
+    return !Check->canBeBypassed(E) && isa<UnaryOperator>(E) &&
            cast<UnaryOperator>(E)->getOpcode() == UO_LNot;
   }
 
+  static bool isExpectedBinaryOp(SimplifyBooleanExprCheck *Check,
+                                 const Expr *E) {
+    const auto *BinaryOp = dyn_cast<BinaryOperator>(E);
+    return !Check->canBeBypassed(E) && BinaryOp && BinaryOp->isLogicalOp() &&
+           BinaryOp->getType()->isBooleanType();
+  }
+
   template <typename Functor>
   static bool checkEitherSide(const BinaryOperator *BO, Functor Func) {
     return Func(BO->getLHS()) || Func(BO->getRHS());
   }
 
-  static bool nestedDemorgan(const Expr *E, unsigned NestingLevel) {
+  bool nestedDemorgan(const Expr *E, unsigned NestingLevel) {
     const auto *BO = dyn_cast<BinaryOperator>(E->IgnoreUnlessSpelledInSource());
     if (!BO)
       return false;
@@ -539,15 +547,14 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
       return true;
     case BO_LAnd:
     case BO_LOr:
-      if (checkEitherSide(BO, isUnaryLNot))
-        return true;
-      if (NestingLevel) {
-        if (checkEitherSide(BO, [NestingLevel](const Expr *E) {
-              return nestedDemorgan(E, NestingLevel - 1);
-            }))
-          return true;
-      }
-      return false;
+      return checkEitherSide(BO,
+                             [this](const Expr *E) {
+                               return isExpectedUnaryLNot(Check, E);
+                             }) ||
+             (NestingLevel &&
+              checkEitherSide(BO, [this, NestingLevel](const Expr *E) {
+                return nestedDemorgan(E, NestingLevel - 1);
+              }));
     default:
       return false;
     }
@@ -556,19 +563,19 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
   bool TraverseUnaryOperator(UnaryOperator *Op) {
     if (!Check->SimplifyDeMorgan || Op->getOpcode() != UO_LNot)
       return Base::TraverseUnaryOperator(Op);
-    Expr *SubImp = Op->getSubExpr()->IgnoreImplicit();
-    auto *Parens = dyn_cast<ParenExpr>(SubImp);
-    auto *BinaryOp =
-        Parens
-            ? dyn_cast<BinaryOperator>(Parens->getSubExpr()->IgnoreImplicit())
-            : dyn_cast<BinaryOperator>(SubImp);
-    if (!BinaryOp || !BinaryOp->isLogicalOp() ||
-        !BinaryOp->getType()->isBooleanType())
+    const Expr *SubImp = Op->getSubExpr()->IgnoreImplicit();
+    const auto *Parens = dyn_cast<ParenExpr>(SubImp);
+    const Expr *SubExpr =
+        Parens ? Parens->getSubExpr()->IgnoreImplicit() : SubImp;
+    if (!isExpectedBinaryOp(Check, SubExpr))
       return Base::TraverseUnaryOperator(Op);
+    const auto *BinaryOp = cast<BinaryOperator>(SubExpr);
     if (Check->SimplifyDeMorganRelaxed ||
-        checkEitherSide(BinaryOp, isUnaryLNot) ||
-        checkEitherSide(BinaryOp,
-                        [](const Expr *E) { return nestedDemorgan(E, 1); })) {
+        checkEitherSide(
+            BinaryOp,
+            [this](const Expr *E) { return isExpectedUnaryLNot(Check, E); }) ||
+        checkEitherSide(
+            BinaryOp, [this](const Expr *E) { return nestedDemorgan(E, 1); })) {
       if (Check->reportDeMorgan(Context, Op, BinaryOp, !IsProcessing, parent(),
                                 Parens) &&
           !Check->areDiagsSelfContained()) {
@@ -694,6 +701,10 @@ void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   Visitor(this, *Result.Context).traverse();
 }
 
+bool SimplifyBooleanExprCheck::canBeBypassed(const Stmt *S) const {
+  return IgnoreMacros && S->getBeginLoc().isMacroID();
+}
+
 void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
                                          SourceLocation Loc,
                                          StringRef Description,

diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -64,6 +64,8 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
                  StringRef Description, SourceRange ReplacementRange,
                  StringRef Replacement);
 
+  bool canBeBypassed(const Stmt *S) const;
+
   const bool IgnoreMacros;
   const bool ChainedConditionalReturn;
   const bool ChainedConditionalAssignment;

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -357,6 +357,9 @@ Changes in existing checks
   support calls to overloaded operators as base expression and provide fixes to
   expressions with side-effects.
 
+- Improved :doc:`readability-simplify-boolean-expr<clang-tidy/checks/readability/simplify-boolean-expr>`
+  check to avoid to emit warning for macro when IgnoreMacro option is enabled.
+
 - Improved :doc:`readability-static-definition-in-anonymous-namespace
   <clang-tidy/checks/readability/static-definition-in-anonymous-namespace>`
   check by resolving fix-it overlaps in template code by disregarding implicit

diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-macros.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-macros.cpp
@@ -6,15 +6,25 @@
 // RUN:     --
 
 #define NEGATE(expr) !(expr)
+#define NOT_AND_NOT(a, b) (!a && !b)
 
 bool without_macro(bool a, bool b) {
     return !(!a && b);
     // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: boolean expression can be simplified by DeMorgan's theorem
     // CHECK-FIXES: return a || !b;
 }
 
-bool macro(bool a, bool b) {
-    return NEGATE(!a && b);
-    // CHECK-MESSAGES-MACROS: :[[@LINE-1]]:12: warning: boolean expression can be simplified by DeMorgan's theorem
-    // CHECK-FIXES: return NEGATE(!a && b);
+void macro(bool a, bool b) {
+    NEGATE(!a && b);
+    // CHECK-MESSAGES-MACROS: :[[@LINE-1]]:5: warning: boolean expression can be simplified by DeMorgan's theorem
+    // CHECK-FIXES: NEGATE(!a && b);
+    !NOT_AND_NOT(a, b);
+    // CHECK-MESSAGES-MACROS: :[[@LINE-1]]:5: warning: boolean expression can be simplified by DeMorgan's theorem
+    // CHECK-FIXES: !NOT_AND_NOT(a, b);
+    !(NEGATE(a) && b);
+    // CHECK-MESSAGES-MACROS: :[[@LINE-1]]:5: warning: boolean expression can be simplified by DeMorgan's theorem
+    // CHECK-FIXES: !(NEGATE(a) && b);
+    !(a && NEGATE(b));
+    // CHECK-MESSAGES-MACROS: :[[@LINE-1]]:5: warning: boolean expression can be simplified by DeMorgan's theorem
+    // CHECK-FIXES: !(a && NEGATE(b));
 }