[clang-tidy] readability-simplify-boolean-expr avoid to warn expression expand from macro when IgnoreMacro option is enabled.
#91757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesFixes: #91487 Full diff: https://github.com/llvm/llvm-project/pull/91757.diff 4 Files Affected:
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..6e85d182b0e1d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -357,6 +357,10 @@ 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 suppression diagnostics from expression expanded from 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));
}
|
|
@llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesFixes: #91487 Full diff: https://github.com/llvm/llvm-project/pull/91757.diff 4 Files Affected:
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..6e85d182b0e1d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -357,6 +357,10 @@ 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 suppression diagnostics from expression expanded from 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));
}
|
HerrCai0907
changed the title
readability-simplify-boolean-expr avoid to warn expression expand from macro when IgnoreMacro option is enabled.
HerrCai0907
requested review from
PiotrZSL,
5chmidti and
SimplyDanny
and removed request for
PiotrZSL and
5chmidti
PiotrZSL
approved these changes
May 10, 2024
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #91487