[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