Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h =================================================================== --- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h +++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -61,6 +61,8 @@ void matchLabelIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value, StringRef Id); + void matchDeMorganExpr(ast_matchers::MatchFinder *Finder); + void replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result, const Expr *BoolLiteral); @@ -77,10 +79,6 @@ const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated); - void - replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result, - const IfStmt *If, bool Negated); - void replaceCompoundReturnWithCondition( const ast_matchers::MatchFinder::MatchResult &Result, const CompoundStmt *Compound, bool Negated); @@ -98,12 +96,22 @@ void replaceLabelCompoundReturnWithCondition( const ast_matchers::MatchFinder::MatchResult &Result, bool Negated); + void + replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result, + const IfStmt *If, bool Negated); + + bool + simplifyDeMorganExpr(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr *OrLeft, bool NotLeft, StringRef Op, + bool NotRight); + void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc, StringRef Description, SourceRange ReplacementRange, StringRef Replacement); const bool ChainedConditionalReturn; const bool ChainedConditionalAssignment; + const bool SimplifyDeMorgan; }; } // namespace readability Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -61,6 +61,14 @@ static constexpr char LabelCompoundBoolId[] = "label-compound-bool"; static constexpr char LabelCompoundNotBoolId[] = "label-compound-bool-not"; static constexpr char IfStmtId[] = "if"; +static constexpr char LeftId[] = "left"; +static constexpr char RightId[] = "right"; +static constexpr char DeMorganOrLeftId[] = "demorgan-or-left"; +static constexpr char DeMorganOrRightId[] = "demorgan-or-right"; +static constexpr char DeMorganOrBothId[] = "demorgan-or-both"; +static constexpr char DeMorganAndLeftId[] = "demorgan-and-left"; +static constexpr char DeMorganAndRightId[] = "demorgan-and-right"; +static constexpr char DeMorganAndBothId[] = "demorgan-and-both"; static constexpr char SimplifyOperatorDiagnostic[] = "redundant boolean literal supplied to boolean operator"; @@ -371,7 +379,8 @@ : ClangTidyCheck(Name, Context), ChainedConditionalReturn(Options.get("ChainedConditionalReturn", false)), ChainedConditionalAssignment( - Options.get("ChainedConditionalAssignment", false)) {} + Options.get("ChainedConditionalAssignment", false)), + SimplifyDeMorgan(Options.get("SimplifyDeMorgan", true)) {} static bool containsBoolLiteral(const Expr *E) { if (!E) @@ -570,10 +579,60 @@ Finder->addMatcher(CompoundStmt, this); } +void SimplifyBooleanExprCheck::matchDeMorganExpr(MatchFinder *Finder) { + if (!SimplifyDeMorgan) + return; + + auto Not = hasOperatorName("!"); + auto Or = hasOperatorName("||"); + auto And = hasOperatorName("&&"); + auto LHSExpr = expr().bind(LeftId); + auto RHSExpr = expr().bind(RightId); + auto NotOp = unaryOperator(Not); + auto LHS = hasLHS(LHSExpr); + auto RHS = hasRHS(RHSExpr); + auto NotLHS = hasLHS(unaryOperator(Not, hasUnaryOperand(LHSExpr))); + auto NotRHS = hasRHS(unaryOperator(Not, hasUnaryOperand(RHSExpr))); + auto UnlessNotRHS = unless(hasRHS(NotOp)); + auto UnlessNotLHS = unless(hasLHS(NotOp)); + // match !(!a || b) + Finder->addMatcher(unaryOperator(Not, hasUnaryOperand(binaryOperator( + Or, UnlessNotRHS, NotLHS, RHS))) + .bind(DeMorganOrLeftId), + this); + // match !(a || !b) + Finder->addMatcher(unaryOperator(Not, hasUnaryOperand(binaryOperator( + Or, UnlessNotLHS, LHS, NotRHS))) + .bind(DeMorganOrRightId), + this); + // match !(!a || !b) + Finder->addMatcher( + unaryOperator(Not, hasUnaryOperand(binaryOperator(Or, NotLHS, NotRHS))) + .bind(DeMorganOrBothId), + this); + + // match !(!a && b) + Finder->addMatcher(unaryOperator(Not, hasUnaryOperand(binaryOperator( + And, UnlessNotRHS, NotLHS, RHS))) + .bind(DeMorganAndLeftId), + this); + // match !(a && !b) + Finder->addMatcher(unaryOperator(Not, hasUnaryOperand(binaryOperator( + And, UnlessNotLHS, LHS, NotRHS))) + .bind(DeMorganAndRightId), + this); + // match !(!a && !b) + Finder->addMatcher( + unaryOperator(Not, hasUnaryOperand(binaryOperator(And, NotLHS, NotRHS))) + .bind(DeMorganAndBothId), + this); +} + void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn); Options.store(Opts, "ChainedConditionalAssignment", ChainedConditionalAssignment); + Options.store(Opts, "SimplifyDeMorgan", SimplifyDeMorgan); } void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { @@ -602,6 +661,8 @@ matchLabelIfReturnsBool(Finder, true, LabelCompoundBoolId); matchLabelIfReturnsBool(Finder, false, LabelCompoundNotBoolId); + + matchDeMorganExpr(Finder); } void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) { @@ -650,6 +711,22 @@ replaceLabelCompoundReturnWithCondition(Result, true); else if (const auto TU = Result.Nodes.getNodeAs("top")) Visitor(this, Result).TraverseDecl(const_cast(TU)); + else if (const auto *OrLeft = Result.Nodes.getNodeAs(DeMorganOrLeftId)) + simplifyDeMorganExpr(Result, OrLeft, false, "&&", true); + else if (const auto *OrRight = + Result.Nodes.getNodeAs(DeMorganOrRightId)) + simplifyDeMorganExpr(Result, OrRight, true, "&&", false); + else if (const auto *OrBoth = Result.Nodes.getNodeAs(DeMorganOrBothId)) + simplifyDeMorganExpr(Result, OrBoth, false, "&&", false); + else if (const auto *AndLeft = + Result.Nodes.getNodeAs(DeMorganAndLeftId)) + simplifyDeMorganExpr(Result, AndLeft, false, "||", true); + else if (const auto *AndRight = + Result.Nodes.getNodeAs(DeMorganAndRightId)) + simplifyDeMorganExpr(Result, AndRight, true, "||", false); + else if (const auto *AndBoth = + Result.Nodes.getNodeAs(DeMorganAndBothId)) + simplifyDeMorganExpr(Result, AndBoth, false, "||", false); } void SimplifyBooleanExprCheck::issueDiag(const MatchFinder::MatchResult &Result, @@ -787,6 +864,26 @@ Replacement); } +bool SimplifyBooleanExprCheck::simplifyDeMorganExpr( + const MatchFinder::MatchResult &Result, const Expr *OrLeft, bool NotLeft, + StringRef Op, bool NotRight) { + if (!SimplifyDeMorgan) + return true; + + const Expr *Left = Result.Nodes.getNodeAs(LeftId); + const Expr *Right = Result.Nodes.getNodeAs(RightId); + std::string Replacement = + (StringRef{"("} + (NotLeft ? "!" : "") + getText(Result, *Left) + " " + + Op + " " + (NotRight ? "!" : "") + getText(Result, *Right) + ")") + .str(); + SourceLocation Loc = OrLeft->getBeginLoc(); + SourceRange Range = OrLeft->getSourceRange(); + issueDiag(Result, Loc, + "boolean expression can be simplified by DeMorgan's theorem", Range, + Replacement); + return false; +} + } // namespace readability } // namespace tidy } // namespace clang Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -136,6 +136,12 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Fixed nonsensical suggestion of :doc:`altera-struct-pack-align + ` check for empty structs. + +- Fixed some false positives in :doc:`bugprone-infinite-loop + ` involving dependent expressions. + - Fixed a crash in :doc:`bugprone-sizeof-expression ` when `sizeof(...)` is compared against a `__int128_t`. @@ -158,26 +164,24 @@ ` involving assignments in conditions. This fixes `Issue 35853 `_. -- Fixed a crash in :doc:`readability-const-return-type - ` when a pure virtual function - overrided has a const return type. Removed the fix for a virtual function. - -- Fixed a false positive in :doc:`readability-non-const-parameter - ` when the parameter is - referenced by an lvalue. - - Improved :doc:`performance-inefficient-vector-operation ` to work when the vector is a member of a structure. -- Fixed nonsensical suggestion of :doc:`altera-struct-pack-align - ` check for empty structs. +- Fixed a crash in :doc:`readability-const-return-type + ` when a pure virtual function + overrided has a const return type. Removed the fix for a virtual function. - Fixed incorrect suggestions for :doc:`readability-container-size-empty ` when smart pointers are involved. -- Fixed some false positives in :doc:`bugprone-infinite-loop - ` involving dependent expressions. +- Fixed a false positive in :doc:`readability-non-const-parameter + ` when the parameter is + referenced by an lvalue. + +- Expanded :doc:`readability-simplify-boolean-expr + ` to simplify expressions + using DeMorgan's Theorem. Removed checks ^^^^^^^^^^^^^^ Index: clang-tools-extra/docs/clang-tidy/checks/readability-simplify-boolean-expr.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/readability-simplify-boolean-expr.rst +++ clang-tools-extra/docs/clang-tidy/checks/readability-simplify-boolean-expr.rst @@ -4,7 +4,8 @@ ================================= Looks for boolean expressions involving boolean constants and simplifies -them to use the appropriate boolean expression directly. +them to use the appropriate boolean expression directly. Simplifies +boolean expressions by application of DeMorgan's Theorem. Examples: @@ -27,6 +28,12 @@ ``if (e) b = false; else b = true;`` ``b = !e;`` ``if (e) return true; return false;`` ``return e;`` ``if (e) return false; return true;`` ``return !e;`` +``!(!a || b)`` ``(a && !b)`` +``!(a || !b)`` ``(!a && b)`` +``!(!a || !b)`` ``(a && b)`` +``!(!a && b)`` ``(a || !b)`` +``!(a && !b)`` ``(!a || b)`` +``!(!a && !b)`` ``(a || b)`` =========================================== ================ The resulting expression ``e`` is modified as follows: @@ -84,3 +91,8 @@ If `true`, conditional boolean assignments at the end of an ``if/else if`` chain will be transformed. Default is `false`. + +.. option:: SimplifyDeMorgan + + If `true`, DeMorgan's Theorem will be applied to simplify negated + conjunctions and disjunctions. Default is `true`. Index: clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-demorgan.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-demorgan.cpp @@ -0,0 +1,24 @@ +// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.SimplifyDeMorgan", value: true}]}" -- + +bool a1 = false; +bool a2 = false; + +bool aa = !(!a1 || a2); +bool ab = !(a1 || !a2); +bool ac = !(!a1 || !a2); +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-FIXES: {{^bool aa = (a1 && !a2);$}} +// CHECK-FIXES-NEXT: {{^bool ab = (!a1 && a2);$}} +// CHECK-FIXES-NEXT: {{^bool ac = (a1 && a2);$}} + +bool ba = !(!a1 && a2); +bool bb = !(a1 && !a2); +bool bc = !(!a1 && !a2); +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr] +// CHECK-FIXES: {{^bool ba = (a1 }}||{{ !a2);$}} +// CHECK-FIXES-NEXT: {{^bool bb = (!a1 }}||{{ a2);$}} +// CHECK-FIXES-NEXT: {{^bool bc = (a1 }}||{{ a2);$}}