From 9e440a185c3f2930067bba6a36f003faeef991ae Mon Sep 17 00:00:00 2001 From: jlallas384 Date: Sun, 19 Oct 2025 09:58:44 +0800 Subject: [PATCH] [clang-tidy] Apply DeMorgan to overloaded operator in the 'readability-simplify-boolean-expr' check This doesn't check whether the negated operator is defined, following the same behavior in other parts of the code that deal with overloaded operators. Fixes #163128 --- .../readability/SimplifyBooleanExprCheck.cpp | 84 +++++++++++++------ clang-tools-extra/docs/ReleaseNotes.rst | 4 + .../simplify-boolean-expr-demorgan.cpp | 15 ++++ 3 files changed, 78 insertions(+), 25 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp index 4184c295b5f0a..50f46ad8459de 100644 --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -809,14 +809,14 @@ void SimplifyBooleanExprCheck::replaceWithAssignment(const ASTContext &Context, Range, Replacement); } -/// Swaps a \c BinaryOperator opcode from `&&` to `||` or vice-versa. +/// Swaps \p BO from `&&` to `||` or vice-versa. static bool flipDemorganOperator(llvm::SmallVectorImpl &Output, - const BinaryOperator *BO) { - assert(BO->isLogicalOp()); - if (BO->getOperatorLoc().isMacroID()) + BinaryOperatorKind BO, SourceLocation BOLoc) { + assert(BinaryOperator::isLogicalOp(BO)); + if (BOLoc.isMacroID()) return true; - Output.push_back(FixItHint::CreateReplacement( - BO->getOperatorLoc(), BO->getOpcode() == BO_LAnd ? "||" : "&&")); + Output.push_back( + FixItHint::CreateReplacement(BOLoc, BO == BO_LAnd ? "||" : "&&")); return false; } @@ -829,22 +829,24 @@ static bool flipDemorganSide(SmallVectorImpl &Fixes, const ASTContext &Ctx, const Expr *E, std::optional OuterBO); -/// Inverts \p BinOp, Removing \p Parens if they exist and are safe to remove. -/// returns \c true if there is any issue building the Fixes, \c false -/// otherwise. +/// Inverts \p BO, \p LHS, and \p RHS, Removing \p Parens if they exist and are +/// safe to remove. returns \c true if there is any issue building the Fixes, \c +/// false otherwise. static bool flipDemorganBinaryOperator(SmallVectorImpl &Fixes, - const ASTContext &Ctx, const BinaryOperator *BinOp, + const ASTContext &Ctx, BinaryOperatorKind BO, + const Expr *LHS, const Expr *RHS, + SourceRange ExprRange, SourceLocation BOLoc, std::optional OuterBO, const ParenExpr *Parens = nullptr) { - switch (BinOp->getOpcode()) { + switch (BO) { case BO_LAnd: case BO_LOr: { // if we have 'a && b' or 'a || b', use demorgan to flip it to '!a || !b' // or '!a && !b'. - if (flipDemorganOperator(Fixes, BinOp)) + if (flipDemorganOperator(Fixes, BO, BOLoc)) return true; - auto NewOp = getDemorganFlippedOperator(BinOp->getOpcode()); + auto NewOp = getDemorganFlippedOperator(BO); if (OuterBO) { // The inner parens are technically needed in a fix for // `!(!A1 && !(A2 || A3)) -> (A1 || (A2 && A3))`, @@ -862,16 +864,16 @@ flipDemorganBinaryOperator(SmallVectorImpl &Fixes, } } if (*OuterBO == BO_LAnd && NewOp == BO_LOr && !Parens) { - Fixes.push_back(FixItHint::CreateInsertion(BinOp->getBeginLoc(), "(")); + Fixes.push_back(FixItHint::CreateInsertion(ExprRange.getBegin(), "(")); Fixes.push_back(FixItHint::CreateInsertion( - Lexer::getLocForEndOfToken(BinOp->getEndLoc(), 0, + Lexer::getLocForEndOfToken(ExprRange.getEnd(), 0, Ctx.getSourceManager(), Ctx.getLangOpts()), ")")); } } - if (flipDemorganSide(Fixes, Ctx, BinOp->getLHS(), NewOp) || - flipDemorganSide(Fixes, Ctx, BinOp->getRHS(), NewOp)) + if (flipDemorganSide(Fixes, Ctx, LHS, NewOp) || + flipDemorganSide(Fixes, Ctx, RHS, NewOp)) return true; return false; }; @@ -882,12 +884,11 @@ flipDemorganBinaryOperator(SmallVectorImpl &Fixes, case BO_EQ: case BO_NE: // For comparison operators, just negate the comparison. - if (BinOp->getOperatorLoc().isMacroID()) + if (BOLoc.isMacroID()) return true; Fixes.push_back(FixItHint::CreateReplacement( - BinOp->getOperatorLoc(), - BinaryOperator::getOpcodeStr( - BinaryOperator::negateComparisonOp(BinOp->getOpcode())))); + BOLoc, + BinaryOperator::getOpcodeStr(BinaryOperator::negateComparisonOp(BO)))); return false; default: // for any other binary operator, just use logical not and wrap in @@ -897,11 +898,11 @@ flipDemorganBinaryOperator(SmallVectorImpl &Fixes, return true; Fixes.push_back(FixItHint::CreateInsertion(Parens->getBeginLoc(), "!")); } else { - if (BinOp->getBeginLoc().isMacroID() || BinOp->getEndLoc().isMacroID()) + if (ExprRange.getBegin().isMacroID() || ExprRange.getEnd().isMacroID()) return true; - Fixes.append({FixItHint::CreateInsertion(BinOp->getBeginLoc(), "!("), + Fixes.append({FixItHint::CreateInsertion(ExprRange.getBegin(), "!("), FixItHint::CreateInsertion( - Lexer::getLocForEndOfToken(BinOp->getEndLoc(), 0, + Lexer::getLocForEndOfToken(ExprRange.getEnd(), 0, Ctx.getSourceManager(), Ctx.getLangOpts()), ")")}); @@ -911,6 +912,28 @@ flipDemorganBinaryOperator(SmallVectorImpl &Fixes, return false; } +static bool +flipDemorganBinaryOperator(SmallVectorImpl &Fixes, + const ASTContext &Ctx, const BinaryOperator *BinOp, + std::optional OuterBO, + const ParenExpr *Parens = nullptr) { + return flipDemorganBinaryOperator( + Fixes, Ctx, BinOp->getOpcode(), BinOp->getLHS(), BinOp->getRHS(), + BinOp->getSourceRange(), BinOp->getOperatorLoc(), OuterBO, Parens); +} + +static bool +flipDemorganOverloadedBinaryOperator(SmallVectorImpl &Fixes, + const ASTContext &Ctx, + const CXXOperatorCallExpr *OpCall, + std::optional OuterBO, + const ParenExpr *Parens = nullptr) { + auto BO = BinaryOperator::getOverloadedOpcode(OpCall->getOperator()); + return flipDemorganBinaryOperator(Fixes, Ctx, BO, OpCall->getArg(0), + OpCall->getArg(1), OpCall->getSourceRange(), + OpCall->getOperatorLoc(), OuterBO, Parens); +} + static bool flipDemorganSide(SmallVectorImpl &Fixes, const ASTContext &Ctx, const Expr *E, std::optional OuterBO) { @@ -930,6 +953,17 @@ static bool flipDemorganSide(SmallVectorImpl &Fixes, return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO, Paren); } } + if (const auto *OpCall = dyn_cast(E); + OpCall && OpCall->isInfixBinaryOp()) { + return flipDemorganOverloadedBinaryOperator(Fixes, Ctx, OpCall, OuterBO); + } + if (const auto *Paren = dyn_cast(E)) { + if (const auto *OpCall = dyn_cast(Paren->getSubExpr()); + OpCall && OpCall->isInfixBinaryOp()) { + return flipDemorganOverloadedBinaryOperator(Fixes, Ctx, OpCall, OuterBO, + Paren); + } + } // Fallback case just insert a logical not operator. if (E->getBeginLoc().isMacroID()) return true; @@ -991,7 +1025,7 @@ bool SimplifyBooleanExprCheck::reportDeMorgan(const ASTContext &Context, } else { Fixes.push_back(FixItHint::CreateRemoval(Outer->getOperatorLoc())); } - if (flipDemorganOperator(Fixes, Inner)) + if (flipDemorganOperator(Fixes, Inner->getOpcode(), Inner->getOperatorLoc())) return false; if (flipDemorganSide(Fixes, Context, Inner->getLHS(), NewOpcode) || flipDemorganSide(Fixes, Context, Inner->getRHS(), NewOpcode)) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6c104bb612a47..0ca8c945a67ca 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -425,6 +425,10 @@ Changes in existing checks ` check to recognize literal suffixes added in C++23 and C23. +- Improved :doc:`readability-simplify-boolean-expr + ` check to handle + overloaded binary operators when applying De Morgan's law. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-demorgan.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-demorgan.cpp index bab9e17a7775b..ccab632e6d0f7 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-demorgan.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr-demorgan.cpp @@ -105,4 +105,19 @@ void foo(bool A1, bool A2, bool A3, bool A4) { // CHECK-FIXES: X = A1 && A2 && A3; // CHECK-FIXES-NEXT: X = A1 || (A2 && A3); // CHECK-FIXES-NEXT: X = A1 && (A2 || A3); + + struct T { + bool operator==(const T&) const; + bool operator<(const T&) const; + }; + T T1, T2; + X = !(T1 == T2 && A1 == A2); + X = !(T1 < T2 || (A1 || !A2)); + X = !((T1 < T2) || (A1 || !A2)); + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: boolean expression can be simplified by DeMorgan's theorem + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: boolean expression can be simplified by DeMorgan's theorem + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: boolean expression can be simplified by DeMorgan's theorem + // CHECK-FIXES: X = T1 != T2 || A1 != A2; + // CHECK-FIXES-NEXT: X = T1 >= T2 && !A1 && A2; + // CHECK-FIXES-NEXT: X = (T1 >= T2) && !A1 && A2; }