llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) <details> <summary>Changes</summary> Fixes: #<!-- -->91487 --- Full diff: https://github.com/llvm/llvm-project/pull/91757.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp (+36-25) - (modified) clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h (+2) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-macros.cpp (+14-4) ``````````diff 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)); } `````````` </details> https://github.com/llvm/llvm-project/pull/91757 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
