llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: PrahladGowtham <details> <summary>Changes</summary> Extend `readability-simplify-boolean-expr` to simplify boolean comparisons inside logical conjunctions (`&&`) where one operand determines the truth value of the variable in the other operand. ### Examples - `a && a != b` -> `a && !b` - `a && a == b` -> `a && b` - `!a && a != b` -> `!a && b` - `!a && a == b` -> `!a && !b` - `a == b && !a` -> `!b && !a` Includes test coverage, documentation updates, and release notes. Closes #<!-- -->232311 --- Full diff: https://github.com/llvm/llvm-project/pull/233080.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp (+78-9) - (modified) clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h (+2) - (modified) clang-tools-extra/docs/ReleaseNotes.md (+5) - (modified) clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.md (+5) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp (+79) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp index 121c327dc3b583..a87abc39e46878 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 "../utils/ASTUtils.h" #include "clang/AST/Expr.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/DiagnosticIDs.h" @@ -38,6 +39,8 @@ static constexpr char SimplifyConditionDiagnostic[] = "redundant boolean literal in if statement condition"; static constexpr char SimplifyConditionalReturnDiagnostic[] = "redundant boolean literal in conditional return statement"; +static constexpr char SimplifyConjunctionDiagnostic[] = + "redundant comparison in boolean expression"; static bool needsParensAfterUnaryNegation(const Expr *E) { E = E->IgnoreImpCasts(); @@ -305,6 +308,7 @@ class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> { bool VisitBinaryOperator(const BinaryOperator *Op) const { Check->reportBinOp(Context, Op); + Check->reportConjunctionWithComparison(Context, Op); return true; } @@ -649,15 +653,14 @@ void SimplifyBooleanExprCheck::reportBinOp(const ASTContext &Context, const bool BoolValue = Bool->getValue(); - const auto ReplaceWithExpression = [this, &Context, LHS, RHS, - Bool](const Expr *ReplaceWith, - bool Negated) { - const std::string Replacement = - replacementExpression(Context, Negated, ReplaceWith); - const SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc()); - issueDiag(Context, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range, - Replacement); - }; + const auto ReplaceWithExpression = + [this, &Context, LHS, RHS, Bool](const Expr *ReplaceWith, bool Negated) { + const std::string Replacement = + replacementExpression(Context, Negated, ReplaceWith); + const SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc()); + issueDiag(Context, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, + Range, Replacement); + }; switch (Op->getOpcode()) { case BO_LAnd: @@ -689,6 +692,72 @@ void SimplifyBooleanExprCheck::reportBinOp(const ASTContext &Context, } } +static std::pair<const Expr *, bool> parseCondition(const Expr *E) { + E = E->IgnoreParenImpCasts(); + if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) { + if (UnOp->getOpcode() == UO_LNot) + return {UnOp->getSubExpr()->IgnoreParenImpCasts(), true}; + } + return {E, false}; +} + +void SimplifyBooleanExprCheck::reportConjunctionWithComparison( + const ASTContext &Context, const BinaryOperator *Op) { + if (canBeBypassed(Op)) + return; + + if (Op->getOpcode() != BO_LAnd || !Op->getType()->isBooleanType()) + return; + + const auto CheckOperands = [this, &Context](const Expr *CondExpr, + const Expr *CompRaw) -> bool { + const auto [ExprA, CondNegated] = parseCondition(CondExpr); + if (!ExprA->getType()->isBooleanType() || ExprA->HasSideEffects(Context)) + return false; + + const Expr *CompUnwrapped = CompRaw->IgnoreParenImpCasts(); + const auto *BinOp = dyn_cast<BinaryOperator>(CompUnwrapped); + if (!BinOp || (BinOp->getOpcode() != BO_EQ && BinOp->getOpcode() != BO_NE)) + return false; + + const Expr *CompLHS = BinOp->getLHS()->IgnoreParenImpCasts(); + const Expr *CompRHS = BinOp->getRHS()->IgnoreParenImpCasts(); + + if (!CompLHS->getType()->isBooleanType() || + !CompRHS->getType()->isBooleanType()) + return false; + + if (CompLHS->HasSideEffects(Context) || CompRHS->HasSideEffects(Context)) + return false; + + if (isa<CXXBoolLiteralExpr>(ExprA) || isa<CXXBoolLiteralExpr>(CompLHS) || + isa<CXXBoolLiteralExpr>(CompRHS)) + return false; + + if (utils::areStatementsIdentical(CompLHS, CompRHS, Context, true)) + return false; + + const Expr *ExprB = nullptr; + if (utils::areStatementsIdentical(CompLHS, ExprA, Context, true)) + ExprB = CompRHS; + else if (utils::areStatementsIdentical(CompRHS, ExprA, Context, true)) + ExprB = CompLHS; + else + return false; + + const bool CompNeedsNegation = (BinOp->getOpcode() == BO_NE) ^ CondNegated; + const std::string Replacement = + replacementExpression(Context, CompNeedsNegation, ExprB); + + return issueDiag(Context, BinOp->getOperatorLoc(), + SimplifyConjunctionDiagnostic, CompRaw->getSourceRange(), + Replacement); + }; + + if (!CheckOperands(Op->getLHS(), Op->getRHS())) + CheckOperands(Op->getRHS(), Op->getLHS()); +} + void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IgnoreMacros", IgnoreMacros); Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn); diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h index d43116b102ce32..83031d1eb313de 100644 --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -36,6 +36,8 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck { class Visitor; void reportBinOp(const ASTContext &Context, const BinaryOperator *Op); + void reportConjunctionWithComparison(const ASTContext &Context, + const BinaryOperator *Op); void replaceWithThenStatement(const ASTContext &Context, const IfStmt *IfStatement, diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 69f3ecee49926b..9b4c8da353df22 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -398,6 +398,11 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/readability/simplify-boolean-expr>` producing invalid fixes when applying De Morgan's theorem to overloaded comparison operators. +- Improved {doc}`readability-simplify-boolean-expr + <clang-tidy/checks/readability/simplify-boolean-expr>` by simplifying + redundant equality comparisons in logical conjunctions (such as `a && a != b` + to `a && !b`, and `a == b && !a` to `!b && !a`). + - Improved {doc}`readability-suspicious-call-argument <clang-tidy/checks/readability/suspicious-call-argument>` check by fixing the default `dist` and `dst` abbreviations of `distance` not being recognized. diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.md b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.md index 12344ce7d71382..83d02674b3fbc4 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.md +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.md @@ -33,6 +33,11 @@ Examples: | `!(!a && b)` | `a \|\| !b` | | `!(a && !b)` | `!a \|\| b` | | `!(!a && !b)` | `a \|\| b` | +| `a && a != b` | `a && !b` | +| `a == b && !a` | `!b && !a` | +| `a && a == b` | `a && b` | +| `!a && a != b` | `!a && b` | +| `!a && a == b` | `!a && !b` | The resulting expression `e` is modified as follows: diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp index 0b99cb89262cdf..978522eec2c0a2 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp @@ -1035,3 +1035,82 @@ void instantiate() { ignoreInstantiations<true>(); ignoreInstantiations<false>(); } + +bool conj_comp1(bool a, bool b) { + return a && a != b; +} +// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return a && !b;{{$}} + +bool conj_comp2(bool a, bool b) { + return a == b && !a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !b && !a;{{$}} + +bool conj_comp3(bool a, bool b) { + return a && a == b; +} +// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return a && b;{{$}} + +bool conj_comp4(bool a, bool b) { + return !a && a != b; +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !a && b;{{$}} + +bool conj_comp5(bool a, bool b) { + return !a && a == b; +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !a && !b;{{$}} + +bool conj_comp6(bool a, bool b) { + return (a != b) && a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !b && a;{{$}} + +bool conj_comp7(bool a, bool b) { + return (a == b) && a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return b && a;{{$}} + +bool conj_comp8(bool a, bool b) { + return (a != b) && !a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return b && !a;{{$}} + +bool conj_comp9(bool a, bool b) { + return b != a && a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !b && a;{{$}} + +bool conj_comp10(bool a, bool b) { + return b == a && !a; +} +// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return !b && !a;{{$}} + +bool conj_comp11(bool a, bool b) { + return a && (a != b); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: redundant comparison in boolean expression [readability-simplify-boolean-expr] +// CHECK-FIXES: return a && !b;{{$}} + +bool conj_comp_neg1(bool a, bool b) { + return a && b; +} + +bool conj_comp_neg2(bool a, bool b, bool c) { + return a && b == c; +} + +bool conj_comp_neg3(int a, int b) { + return a && a != b; +} + `````````` </details> https://github.com/llvm/llvm-project/pull/233080 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
