[clang-tools-extra] [clang-tidy] Apply DeMorgan to overloaded operator in the 'readability-simplify-boolean-expr' check (PR #164141)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 19 19:27:40 PDT 2025
https://github.com/jlallas384 updated https://github.com/llvm/llvm-project/pull/164141
>From 9e440a185c3f2930067bba6a36f003faeef991ae Mon Sep 17 00:00:00 2001
From: jlallas384 <jlallas384 at gmail.com>
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<FixItHint> &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<FixItHint> &Fixes,
const ASTContext &Ctx, const Expr *E,
std::optional<BinaryOperatorKind> 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<FixItHint> &Fixes,
- const ASTContext &Ctx, const BinaryOperator *BinOp,
+ const ASTContext &Ctx, BinaryOperatorKind BO,
+ const Expr *LHS, const Expr *RHS,
+ SourceRange ExprRange, SourceLocation BOLoc,
std::optional<BinaryOperatorKind> 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<FixItHint> &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<FixItHint> &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<FixItHint> &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<FixItHint> &Fixes,
return false;
}
+static bool
+flipDemorganBinaryOperator(SmallVectorImpl<FixItHint> &Fixes,
+ const ASTContext &Ctx, const BinaryOperator *BinOp,
+ std::optional<BinaryOperatorKind> 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<FixItHint> &Fixes,
+ const ASTContext &Ctx,
+ const CXXOperatorCallExpr *OpCall,
+ std::optional<BinaryOperatorKind> 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<FixItHint> &Fixes,
const ASTContext &Ctx, const Expr *E,
std::optional<BinaryOperatorKind> OuterBO) {
@@ -930,6 +953,17 @@ static bool flipDemorganSide(SmallVectorImpl<FixItHint> &Fixes,
return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO, Paren);
}
}
+ if (const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(E);
+ OpCall && OpCall->isInfixBinaryOp()) {
+ return flipDemorganOverloadedBinaryOperator(Fixes, Ctx, OpCall, OuterBO);
+ }
+ if (const auto *Paren = dyn_cast<ParenExpr>(E)) {
+ if (const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(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
<clang-tidy/checks/readability/uppercase-literal-suffix>` check to recognize
literal suffixes added in C++23 and C23.
+- Improved :doc:`readability-simplify-boolean-expr
+ <clang-tidy/checks/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;
}
More information about the cfe-commits
mailing list