[clang-tools-extra] [clang-tidy] `readability-simplify-boolean-expr` avoid to warn expression expand from macro when ``IgnoreMacro`` option is enabled. (PR #91757)

Congcong Cai via cfe-commits cfe-commits at lists.llvm.org
Fri May 10 16:20:43 PDT 2024


https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/91757

>From e23675eae31ed9c3cc4bbf31d3bc43b492ef5eac Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Thu, 9 May 2024 00:51:58 +0800
Subject: [PATCH 1/2] [clang-tidy] avoid false postive when ignore macro

Fixes: #91487
---
 .../readability/SimplifyBooleanExprCheck.cpp  | 61 +++++++++++--------
 .../readability/SimplifyBooleanExprCheck.h    |  2 +
 clang-tools-extra/docs/ReleaseNotes.rst       |  3 +
 .../simplify-boolean-expr-macros.cpp          | 18 ++++--
 4 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index edb67614bd558..4b4ffc3fe0074 100644
--- 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
index ccc6f3d879fc0..63c3caa01e01a 100644
--- 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
index 3f0d25ec8c752..6ed1ad40b452c 100644
--- 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
index 7d0cfe7e27dc2..d1df79e23a1e6 100644
--- 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,6 +6,7 @@
 // RUN:     --
 
 #define NEGATE(expr) !(expr)
+#define NOT_AND_NOT(a, b) (!a && !b)
 
 bool without_macro(bool a, bool b) {
     return !(!a && b);
@@ -13,8 +14,17 @@ bool without_macro(bool a, bool b) {
     // 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));
 }

>From a8b33f0287cb01a58f3577aa01048d49e60cd43e Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Sat, 11 May 2024 07:20:26 +0800
Subject: [PATCH 2/2] fix

---
 .../readability/SimplifyBooleanExprCheck.cpp    | 17 +++++++----------
 clang-tools-extra/docs/ReleaseNotes.rst         |  5 +++--
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index 4b4ffc3fe0074..fd4730d9c8b9c 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -513,14 +513,12 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
     return true;
   }
 
-  static bool isExpectedUnaryLNot(SimplifyBooleanExprCheck *Check,
-                                  const Expr *E) {
+  bool isExpectedUnaryLNot(const Expr *E) {
     return !Check->canBeBypassed(E) && isa<UnaryOperator>(E) &&
            cast<UnaryOperator>(E)->getOpcode() == UO_LNot;
   }
 
-  static bool isExpectedBinaryOp(SimplifyBooleanExprCheck *Check,
-                                 const Expr *E) {
+  bool isExpectedBinaryOp(const Expr *E) {
     const auto *BinaryOp = dyn_cast<BinaryOperator>(E);
     return !Check->canBeBypassed(E) && BinaryOp && BinaryOp->isLogicalOp() &&
            BinaryOp->getType()->isBooleanType();
@@ -547,10 +545,9 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
       return true;
     case BO_LAnd:
     case BO_LOr:
-      return checkEitherSide(BO,
-                             [this](const Expr *E) {
-                               return isExpectedUnaryLNot(Check, E);
-                             }) ||
+      return checkEitherSide(
+                 BO,
+                 [this](const Expr *E) { return isExpectedUnaryLNot(E); }) ||
              (NestingLevel &&
               checkEitherSide(BO, [this, NestingLevel](const Expr *E) {
                 return nestedDemorgan(E, NestingLevel - 1);
@@ -567,13 +564,13 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
     const auto *Parens = dyn_cast<ParenExpr>(SubImp);
     const Expr *SubExpr =
         Parens ? Parens->getSubExpr()->IgnoreImplicit() : SubImp;
-    if (!isExpectedBinaryOp(Check, SubExpr))
+    if (!isExpectedBinaryOp(SubExpr))
       return Base::TraverseUnaryOperator(Op);
     const auto *BinaryOp = cast<BinaryOperator>(SubExpr);
     if (Check->SimplifyDeMorganRelaxed ||
         checkEitherSide(
             BinaryOp,
-            [this](const Expr *E) { return isExpectedUnaryLNot(Check, E); }) ||
+            [this](const Expr *E) { return isExpectedUnaryLNot(E); }) ||
         checkEitherSide(
             BinaryOp, [this](const Expr *E) { return nestedDemorgan(E, 1); })) {
       if (Check->reportDeMorgan(Context, Op, BinaryOp, !IsProcessing, parent(),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6ed1ad40b452c..6a2b8d3b6ded6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -357,8 +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-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>`



More information about the cfe-commits mailing list