njames93 updated this revision to Diff 427038.
njames93 added a comment.
Enhanced support for parens, both adding them in when needed as well as
removing some un-needed parens.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124806/new/
https://reviews.llvm.org/D124806
Files:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
clang-tools-extra/docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-demorgan.cpp
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,64 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+void foo(bool A1, bool A2, bool A3) {
+ bool X;
+ X = !(!A1 || A2);
+ X = !(A1 || !A2);
+ X = !(!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 = (A1 && !A2);
+ // CHECK-FIXES-NEXT: X = (!A1 && A2);
+ // CHECK-FIXES-NEXT: X = (A1 && A2);
+
+ X = !(!A1 && A2);
+ X = !(A1 && !A2);
+ X = !(!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 = (A1 || !A2);
+ // CHECK-FIXES-NEXT: X = (!A1 || A2);
+ // CHECK-FIXES-NEXT: X = (A1 || A2);
+
+ X = !(!A1 && !A2 && !A3);
+ X = !(!A1 && (!A2 && !A3));
+ X = !(!A1 && (A2 && A3));
+ // 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 = (A1 || A2 || A3);
+ // CHECK-FIXES-NEXT: X = (A1 || A2 || A3);
+ // CHECK-FIXES-NEXT: X = (A1 || !A2 || !A3);
+
+ X = !(A1 && A2 == A3);
+ X = !(!A1 && A2 > A3);
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-FIXES: X = (!A1 || A2 != A3);
+ // CHECK-FIXES-NEXT: X = (A1 || A2 <= A3);
+
+ // Ensure the check doesn't try to combine fixes for the inner and outer demorgan simplification.
+ X = !(!A1 && !(!A2 && !A3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-FIXES: X = (A1 || (!A2 && !A3));
+
+ // Testing to see how it handles parens
+ X = !(A1 && !A2 && !A3);
+ X = !(A1 && !A2 || !A3);
+ X = !(!A1 || A2 && !A3);
+ X = !((A1 || !A2) && !A3);
+ X = !((A1 || !A2) || !A3);
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: boolean expression can be simplified by DeMorgan's theorem
+ // CHECK-FIXES: X = (!A1 || A2 || A3);
+ // CHECK-FIXES-NEXT: X = ((!A1 || A2) && A3);
+ // CHECK-FIXES-NEXT: X = (A1 && (!A2 || A3));
+ // CHECK-FIXES-NEXT: X = (!A1 && A2 || A3);
+ // CHECK-FIXES-NEXT: X = (!A1 && A2 && A3);
+}
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/clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H
#include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
namespace clang {
namespace tidy {
@@ -98,12 +99,17 @@
void replaceLabelCompoundReturnWithCondition(
const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
+ void replaceDeMorgan(const ASTContext &Context, const UnaryOperator *Outer,
+ const BinaryOperator *Inner);
+
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;
+ llvm::DenseSet<const UnaryOperator *> DemorganOps;
};
} // 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
@@ -10,6 +10,8 @@
#include "SimplifyBooleanExprMatchers.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
#include <string>
#include <utility>
@@ -61,6 +63,8 @@
static constexpr char LabelCompoundBoolId[] = "label-compound-bool";
static constexpr char LabelCompoundNotBoolId[] = "label-compound-bool-not";
static constexpr char IfStmtId[] = "if";
+static constexpr char DeMorganID[] = "demorgan";
+static constexpr char DemorganInnerID[] = "demorganInner";
static constexpr char SimplifyOperatorDiagnostic[] =
"redundant boolean literal supplied to boolean operator";
@@ -371,7 +375,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)
@@ -576,6 +581,22 @@
ChainedConditionalAssignment);
}
+namespace {
+AST_MATCHER(BinaryOperator, isNegateableComparisonOperator) {
+ switch (Node.getOpcode()) {
+ case BO_LT:
+ case BO_GT:
+ case BO_LE:
+ case BO_GE:
+ case BO_EQ:
+ case BO_NE:
+ return true;
+ default:
+ return false;
+ }
+}
+} // namespace
+
void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(translationUnitDecl().bind("top"), this);
@@ -602,6 +623,21 @@
matchLabelIfReturnsBool(Finder, true, LabelCompoundBoolId);
matchLabelIfReturnsBool(Finder, false, LabelCompoundNotBoolId);
+
+ if (SimplifyDeMorgan) {
+ Finder->addMatcher(
+ unaryOperator(
+ hasOperatorName("!"),
+ hasUnaryOperand(ignoringParens(
+ binaryOperator(
+ hasAnyOperatorName("&&", "||"), hasType(booleanType()),
+ hasEitherOperand(anyOf(
+ unaryOperator(hasOperatorName("!")),
+ binaryOperator(isNegateableComparisonOperator()))))
+ .bind(DemorganInnerID))))
+ .bind(DeMorganID),
+ this);
+ }
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
@@ -650,6 +686,9 @@
replaceLabelCompoundReturnWithCondition(Result, true);
else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
+ else if (const auto *DMT = Result.Nodes.getNodeAs<UnaryOperator>(DeMorganID))
+ replaceDeMorgan(*Result.Context, DMT,
+ Result.Nodes.getNodeAs<BinaryOperator>(DemorganInnerID));
}
void SimplifyBooleanExprCheck::issueDiag(const MatchFinder::MatchResult &Result,
@@ -787,6 +826,162 @@
Replacement);
}
+/// Swaps a \c BinaryOperator opcode from `&&` to `||` or vice-versa.
+static bool flipDemorganOperator(llvm::SmallVectorImpl<FixItHint> &Output,
+ const BinaryOperator *BO) {
+ assert(BO->getOpcode() == BO_LAnd || BO->getOpcode() == BO_LOr);
+ if (BO->getOperatorLoc().isMacroID())
+ return true;
+ Output.push_back(FixItHint::CreateReplacement(
+ BO->getOperatorLoc(), BO->getOpcode() == BO_LAnd ? "||" : "&&"));
+ return false;
+}
+
+static BinaryOperatorKind getDemorganFlippedOperator(BinaryOperatorKind BO) {
+ assert(BO == BO_LAnd || BO == BO_LOr);
+ return BO == BO_LAnd ? BO_LOr : BO_LAnd;
+}
+
+namespace {
+struct DmFixesInfo {
+ const ASTContext &Ctx;
+ llvm::SmallVector<FixItHint, 8> Fixes;
+ llvm::SmallVector<const UnaryOperator *> Tracker;
+};
+} // namespace
+
+static bool flipDemorganSide(DmFixesInfo &Fix, const Expr *E,
+ Optional<BinaryOperatorKind> OuterBO);
+
+static SourceLocation getEndOfTok(const ASTContext &Context,
+ SourceLocation Loc) {
+ return Loc.getLocWithOffset(Lexer::MeasureTokenLength(
+ Loc, Context.getSourceManager(), Context.getLangOpts()));
+}
+
+static bool flipDemorganBinaryOperator(DmFixesInfo &Fix,
+ const BinaryOperator *BinOp,
+ Optional<BinaryOperatorKind> OuterBO,
+ const ParenExpr *Parens = nullptr) {
+ switch (BinOp->getOpcode()) {
+ 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(Fix.Fixes, BinOp))
+ return true;
+ auto NewOp = getDemorganFlippedOperator(BinOp->getOpcode());
+ if (OuterBO) {
+ if (((*OuterBO == NewOp) || (*OuterBO == BO_LOr && NewOp == BO_LAnd)) &&
+ Parens) {
+ if (!Parens->getLParen().isMacroID() &&
+ !Parens->getRParen().isMacroID()) {
+ Fix.Fixes.push_back(FixItHint::CreateRemoval(Parens->getLParen()));
+ Fix.Fixes.push_back(FixItHint::CreateRemoval(Parens->getRParen()));
+ }
+ }
+ if (*OuterBO == BO_LAnd && NewOp == BO_LOr && !Parens) {
+ Fix.Fixes.push_back(
+ FixItHint::CreateInsertion(BinOp->getBeginLoc(), "("));
+ Fix.Fixes.push_back(FixItHint::CreateInsertion(
+ getEndOfTok(Fix.Ctx, BinOp->getEndLoc()), ")"));
+ }
+ }
+ if (flipDemorganSide(Fix, BinOp->getLHS(), NewOp) ||
+ flipDemorganSide(Fix, BinOp->getRHS(), NewOp))
+ return true;
+ return false;
+ };
+ case BO_LT:
+ case BO_GT:
+ case BO_LE:
+ case BO_GE:
+ case BO_EQ:
+ case BO_NE:
+ // For comparison operators, just negate the comparison.
+ if (BinOp->getOperatorLoc().isMacroID())
+ return true;
+ Fix.Fixes.push_back(FixItHint::CreateReplacement(
+ BinOp->getOperatorLoc(),
+ BinaryOperator::getOpcodeStr(
+ BinaryOperator::negateComparisonOp(BinOp->getOpcode()))));
+ return false;
+ default:
+ // for any other binary operator, just use logical not and wrap in
+ // parens.
+ if (Parens) {
+ if (Parens->getBeginLoc().isMacroID())
+ return true;
+ Fix.Fixes.push_back(
+ FixItHint::CreateInsertion(Parens->getBeginLoc(), "!"));
+ } else {
+ if (BinOp->getBeginLoc().isMacroID() || BinOp->getEndLoc().isMacroID())
+ return true;
+ Fix.Fixes.append({FixItHint::CreateInsertion(BinOp->getBeginLoc(), "!("),
+ FixItHint::CreateInsertion(
+ getEndOfTok(Fix.Ctx, BinOp->getEndLoc()), ")")});
+ }
+ break;
+ }
+ return false;
+}
+
+static bool flipDemorganSide(DmFixesInfo &Fix, const Expr *E,
+ Optional<BinaryOperatorKind> OuterBO) {
+ if (isa<UnaryOperator>(E) && cast<UnaryOperator>(E)->getOpcode() == UO_LNot) {
+ Fix.Tracker.push_back(cast<UnaryOperator>(E));
+ // if we have a not operator, '!a', just remove the '!'.
+ if (cast<UnaryOperator>(E)->getOperatorLoc().isMacroID())
+ return true;
+ Fix.Fixes.push_back(
+ FixItHint::CreateRemoval(cast<UnaryOperator>(E)->getOperatorLoc()));
+ return false;
+ }
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+ return flipDemorganBinaryOperator(Fix, BinOp, OuterBO);
+ }
+ if (const auto *Paren = dyn_cast<ParenExpr>(E)) {
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(Paren->getSubExpr())) {
+ return flipDemorganBinaryOperator(Fix, BinOp, OuterBO, Paren);
+ }
+ }
+ // Fallback case just insert a logical not operator.
+ if (E->getBeginLoc().isMacroID())
+ return true;
+ Fix.Fixes.push_back(FixItHint::CreateInsertion(E->getBeginLoc(), "!"));
+ return false;
+}
+
+void SimplifyBooleanExprCheck::replaceDeMorgan(const ASTContext &Context,
+ const UnaryOperator *Outer,
+ const BinaryOperator *Inner) {
+ assert(Outer);
+ assert(Inner);
+ assert(Inner->getOpcode() == BO_LAnd || Inner->getOpcode() == BO_LOr);
+
+ auto Diag =
+ diag(Outer->getBeginLoc(),
+ "boolean expression can be simplified by DeMorgan's theorem");
+ Diag << Outer->getSourceRange();
+ // If we have already fixed this with a previous fix, don't attempt any fixes
+ if (!areDiagsSelfContained() && !DemorganOps.insert(Outer).second)
+ return;
+ if (Outer->getOperatorLoc().isMacroID())
+ return;
+ DmFixesInfo Fix{Context};
+ Fix.Fixes.push_back(FixItHint::CreateRemoval(Outer->getOperatorLoc()));
+ if (flipDemorganOperator(Fix.Fixes, Inner))
+ return;
+ auto NewOperator = getDemorganFlippedOperator(Inner->getOpcode());
+ if (flipDemorganSide(Fix, Inner->getLHS(), NewOperator))
+ return;
+ if (flipDemorganSide(Fix, Inner->getRHS(), NewOperator))
+ return;
+ Diag << Fix.Fixes;
+ if (!areDiagsSelfContained()) {
+ DemorganOps.insert(Fix.Tracker.begin(), Fix.Tracker.end());
+ }
+}
} // namespace readability
} // namespace tidy
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
