[clang-tools-extra] 99217fa - [clang-tidy] Recognize labelled statements when simplifying boolean exprs
via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 28 15:10:00 PST 2022
Author: Richard
Date: 2022-01-28T16:09:46-07:00
New Revision: 99217fa8a027a893a9b2f46ed315ec4cab850e3d
URL: https://github.com/llvm/llvm-project/commit/99217fa8a027a893a9b2f46ed315ec4cab850e3d
DIFF: https://github.com/llvm/llvm-project/commit/99217fa8a027a893a9b2f46ed315ec4cab850e3d.diff
LOG: [clang-tidy] Recognize labelled statements when simplifying boolean exprs
Inside a switch the caseStmt() and defaultStmt() have a nested statement
associated with them. Similarly, labelStmt() has a nested statement.
These statements were being missed when looking for a compound-if of the
form "if (x) return true; return false;" when the if is nested under one
of these labelling constructs.
Enhance the matchers to look for these nested statements using some
private matcher hasSubstatement() traversal matcher on case, default
and label statements. Add the private matcher hasSubstatementSequence()
to match the compound "if (x) return true; return false;" pattern.
- Add unit tests for private matchers and corresponding test
infrastructure
- Add corresponding test file readability-simplify-bool-expr-case.cpp.
- Fix variable name copy/paste error in readability-simplify-bool-expr.cpp.
- Drop the asserts, which were used only for debugging matchers.
- Run clang-format on the whole check.
- Move local functions out of anonymous namespace and declare state, per
LLVM style guide
- Declare labels constexpr
- Declare visitor arguments as pointer to const
- Drop braces around simple control statements per LLVM style guide
- Prefer explicit arguments over default arguments to methods
Differential Revision: https://reviews.llvm.org/D56303
Fixes #27078
Added:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp
Modified:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index 4ea8ef65d3f8d..9c2ddf139a128 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -7,10 +7,10 @@
//===----------------------------------------------------------------------===//
#include "SimplifyBooleanExprCheck.h"
+#include "SimplifyBooleanExprMatchers.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Lex/Lexer.h"
-#include <cassert>
#include <string>
#include <utility>
@@ -33,33 +33,44 @@ StringRef getText(const MatchFinder::MatchResult &Result, T &Node) {
return getText(Result, Node.getSourceRange());
}
-const char ConditionThenStmtId[] = "if-bool-yields-then";
-const char ConditionElseStmtId[] = "if-bool-yields-else";
-const char TernaryId[] = "ternary-bool-yields-condition";
-const char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
-const char IfReturnsBoolId[] = "if-return";
-const char IfReturnsNotBoolId[] = "if-not-return";
-const char ThenLiteralId[] = "then-literal";
-const char IfAssignVariableId[] = "if-assign-lvalue";
-const char IfAssignLocId[] = "if-assign-loc";
-const char IfAssignBoolId[] = "if-assign";
-const char IfAssignNotBoolId[] = "if-assign-not";
-const char IfAssignVarId[] = "if-assign-var";
-const char CompoundReturnId[] = "compound-return";
-const char CompoundBoolId[] = "compound-bool";
-const char CompoundNotBoolId[] = "compound-bool-not";
-
-const char IfStmtId[] = "if";
-
-const char SimplifyOperatorDiagnostic[] =
+} // namespace
+
+static constexpr char ConditionThenStmtId[] = "if-bool-yields-then";
+static constexpr char ConditionElseStmtId[] = "if-bool-yields-else";
+static constexpr char TernaryId[] = "ternary-bool-yields-condition";
+static constexpr char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
+static constexpr char IfReturnsBoolId[] = "if-return";
+static constexpr char IfReturnsNotBoolId[] = "if-not-return";
+static constexpr char ThenLiteralId[] = "then-literal";
+static constexpr char IfAssignVariableId[] = "if-assign-lvalue";
+static constexpr char IfAssignLocId[] = "if-assign-loc";
+static constexpr char IfAssignBoolId[] = "if-assign";
+static constexpr char IfAssignNotBoolId[] = "if-assign-not";
+static constexpr char IfAssignVarId[] = "if-assign-var";
+static constexpr char CompoundReturnId[] = "compound-return";
+static constexpr char CompoundIfId[] = "compound-if";
+static constexpr char CompoundBoolId[] = "compound-bool";
+static constexpr char CompoundNotBoolId[] = "compound-bool-not";
+static constexpr char CaseId[] = "case";
+static constexpr char CaseCompoundBoolId[] = "case-compound-bool";
+static constexpr char CaseCompoundNotBoolId[] = "case-compound-bool-not";
+static constexpr char DefaultId[] = "default";
+static constexpr char DefaultCompoundBoolId[] = "default-compound-bool";
+static constexpr char DefaultCompoundNotBoolId[] = "default-compound-bool-not";
+static constexpr char LabelId[] = "label";
+static constexpr char LabelCompoundBoolId[] = "label-compound-bool";
+static constexpr char LabelCompoundNotBoolId[] = "label-compound-bool-not";
+static constexpr char IfStmtId[] = "if";
+
+static constexpr char SimplifyOperatorDiagnostic[] =
"redundant boolean literal supplied to boolean operator";
-const char SimplifyConditionDiagnostic[] =
+static constexpr char SimplifyConditionDiagnostic[] =
"redundant boolean literal in if statement condition";
-const char SimplifyConditionalReturnDiagnostic[] =
+static constexpr char SimplifyConditionalReturnDiagnostic[] =
"redundant boolean literal in conditional return statement";
-const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
- StringRef Id) {
+static const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
+ StringRef Id) {
if (const Expr *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id))
return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
if (const auto *Negated = Result.Nodes.getNodeAs<UnaryOperator>(Id)) {
@@ -70,21 +81,22 @@ const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
return nullptr;
}
-internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
+static internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
return expr(
anyOf(cxxBoolLiteral(equals(Value)),
unaryOperator(hasUnaryOperand(cxxBoolLiteral(equals(!Value))),
hasOperatorName("!"))));
}
-internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
+static internal::Matcher<Stmt> returnsBool(bool Value,
+ StringRef Id = "ignored") {
auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
.bind("returns-bool");
return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
}
-bool needsParensAfterUnaryNegation(const Expr *E) {
+static bool needsParensAfterUnaryNegation(const Expr *E) {
E = E->IgnoreImpCasts();
if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
return true;
@@ -96,10 +108,10 @@ bool needsParensAfterUnaryNegation(const Expr *E) {
return false;
}
-std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
+static std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
{BO_LT, BO_GE}, {BO_GT, BO_LE}, {BO_EQ, BO_NE}};
-StringRef negatedOperator(const BinaryOperator *BinOp) {
+static StringRef negatedOperator(const BinaryOperator *BinOp) {
const BinaryOperatorKind Opcode = BinOp->getOpcode();
for (auto NegatableOp : Opposites) {
if (Opcode == NegatableOp.first)
@@ -107,28 +119,28 @@ StringRef negatedOperator(const BinaryOperator *BinOp) {
if (Opcode == NegatableOp.second)
return BinOp->getOpcodeStr(NegatableOp.first);
}
- return StringRef();
+ return {};
}
-std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
+static std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
{OO_EqualEqual, "=="}, {OO_ExclaimEqual, "!="}, {OO_Less, "<"},
{OO_GreaterEqual, ">="}, {OO_Greater, ">"}, {OO_LessEqual, "<="}};
-StringRef getOperatorName(OverloadedOperatorKind OpKind) {
+static StringRef getOperatorName(OverloadedOperatorKind OpKind) {
for (auto Name : OperatorNames) {
if (Name.first == OpKind)
return Name.second;
}
- return StringRef();
+ return {};
}
-std::pair<OverloadedOperatorKind, OverloadedOperatorKind> OppositeOverloads[] =
- {{OO_EqualEqual, OO_ExclaimEqual},
- {OO_Less, OO_GreaterEqual},
- {OO_Greater, OO_LessEqual}};
+static std::pair<OverloadedOperatorKind, OverloadedOperatorKind>
+ OppositeOverloads[] = {{OO_EqualEqual, OO_ExclaimEqual},
+ {OO_Less, OO_GreaterEqual},
+ {OO_Greater, OO_LessEqual}};
-StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
+static StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
const OverloadedOperatorKind Opcode = OpCall->getOperator();
for (auto NegatableOp : OppositeOverloads) {
if (Opcode == NegatableOp.first)
@@ -136,17 +148,17 @@ StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
if (Opcode == NegatableOp.second)
return getOperatorName(NegatableOp.first);
}
- return StringRef();
+ return {};
}
-std::string asBool(StringRef Text, bool NeedsStaticCast) {
+static std::string asBool(StringRef Text, bool NeedsStaticCast) {
if (NeedsStaticCast)
return ("static_cast<bool>(" + Text + ")").str();
return std::string(Text);
}
-bool needsNullPtrComparison(const Expr *E) {
+static bool needsNullPtrComparison(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
return ImpCast->getCastKind() == CK_PointerToBoolean ||
ImpCast->getCastKind() == CK_MemberPointerToBoolean;
@@ -154,14 +166,14 @@ bool needsNullPtrComparison(const Expr *E) {
return false;
}
-bool needsZeroComparison(const Expr *E) {
+static bool needsZeroComparison(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
return ImpCast->getCastKind() == CK_IntegralToBoolean;
return false;
}
-bool needsStaticCast(const Expr *E) {
+static bool needsStaticCast(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
ImpCast->getSubExpr()->getType()->isBooleanType()) {
@@ -180,9 +192,9 @@ bool needsStaticCast(const Expr *E) {
return !E->getType()->isBooleanType();
}
-std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated,
- const char *Constant) {
+static std::string
+compareExpressionToConstant(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated, const char *Constant) {
E = E->IgnoreImpCasts();
const std::string ExprText =
(isa<BinaryOperator>(E) ? ("(" + getText(Result, *E) + ")")
@@ -191,20 +203,22 @@ std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result,
return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant;
}
-std::string compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated) {
+static std::string
+compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated) {
const char *NullPtr =
Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
return compareExpressionToConstant(Result, E, Negated, NullPtr);
}
-std::string compareExpressionToZero(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated) {
+static std::string
+compareExpressionToZero(const MatchFinder::MatchResult &Result, const Expr *E,
+ bool Negated) {
return compareExpressionToConstant(Result, E, Negated, "0");
}
-std::string replacementExpression(const MatchFinder::MatchResult &Result,
- bool Negated, const Expr *E) {
+static std::string replacementExpression(const MatchFinder::MatchResult &Result,
+ bool Negated, const Expr *E) {
E = E->IgnoreParenBaseCasts();
if (const auto *EC = dyn_cast<ExprWithCleanups>(E))
E = EC->getSubExpr();
@@ -281,7 +295,7 @@ std::string replacementExpression(const MatchFinder::MatchResult &Result,
return asBool(getText(Result, *E), NeedsStaticCast);
}
-const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+static const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
if (Bool->getValue() == !Negated)
return Bool;
@@ -299,7 +313,7 @@ const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
return nullptr;
}
-const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
+static const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
if (IfRet->getElse() != nullptr)
return nullptr;
@@ -316,8 +330,8 @@ const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
return nullptr;
}
-bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
- CharSourceRange CharRange) {
+static bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
+ CharSourceRange CharRange) {
std::string ReplacementText =
Lexer::getSourceText(CharRange, *Result.SourceManager,
Result.Context->getLangOpts())
@@ -336,20 +350,18 @@ bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
return false;
}
-} // namespace
-
class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
- public:
+public:
Visitor(SimplifyBooleanExprCheck *Check,
const MatchFinder::MatchResult &Result)
: Check(Check), Result(Result) {}
- bool VisitBinaryOperator(BinaryOperator *Op) {
+ bool VisitBinaryOperator(const BinaryOperator *Op) const {
Check->reportBinOp(Result, Op);
return true;
}
- private:
+private:
SimplifyBooleanExprCheck *Check;
const MatchFinder::MatchResult &Result;
};
@@ -361,7 +373,7 @@ SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
ChainedConditionalAssignment(
Options.get("ChainedConditionalAssignment", false)) {}
-bool containsBoolLiteral(const Expr *E) {
+static bool containsBoolLiteral(const Expr *E) {
if (!E)
return false;
E = E->IgnoreParenImpCasts();
@@ -381,10 +393,10 @@ void SimplifyBooleanExprCheck::reportBinOp(
const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
const CXXBoolLiteralExpr *Bool;
- const Expr *Other = nullptr;
- if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)))
+ const Expr *Other;
+ if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)) != nullptr)
Other = RHS;
- else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)))
+ else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)) != nullptr)
Other = LHS;
else
return;
@@ -408,34 +420,32 @@ void SimplifyBooleanExprCheck::reportBinOp(
};
switch (Op->getOpcode()) {
- case BO_LAnd:
- if (BoolValue) {
- // expr && true -> expr
- ReplaceWithExpression(Other, /*Negated=*/false);
- } else {
- // expr && false -> false
- ReplaceWithExpression(Bool, /*Negated=*/false);
- }
- break;
- case BO_LOr:
- if (BoolValue) {
- // expr || true -> true
- ReplaceWithExpression(Bool, /*Negated=*/false);
- } else {
- // expr || false -> expr
- ReplaceWithExpression(Other, /*Negated=*/false);
- }
- break;
- case BO_EQ:
- // expr == true -> expr, expr == false -> !expr
- ReplaceWithExpression(Other, /*Negated=*/!BoolValue);
- break;
- case BO_NE:
- // expr != true -> !expr, expr != false -> expr
- ReplaceWithExpression(Other, /*Negated=*/BoolValue);
- break;
- default:
- break;
+ case BO_LAnd:
+ if (BoolValue)
+ // expr && true -> expr
+ ReplaceWithExpression(Other, /*Negated=*/false);
+ else
+ // expr && false -> false
+ ReplaceWithExpression(Bool, /*Negated=*/false);
+ break;
+ case BO_LOr:
+ if (BoolValue)
+ // expr || true -> true
+ ReplaceWithExpression(Bool, /*Negated=*/false);
+ else
+ // expr || false -> expr
+ ReplaceWithExpression(Other, /*Negated=*/false);
+ break;
+ case BO_EQ:
+ // expr == true -> expr, expr == false -> !expr
+ ReplaceWithExpression(Other, /*Negated=*/!BoolValue);
+ break;
+ case BO_NE:
+ // expr != true -> !expr, expr != false -> expr
+ ReplaceWithExpression(Other, /*Negated=*/BoolValue);
+ break;
+ default:
+ break;
}
}
@@ -449,12 +459,11 @@ void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
}
void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
- bool Value,
- StringRef TernaryId) {
+ bool Value, StringRef Id) {
Finder->addMatcher(
conditionalOperator(hasTrueExpression(literalOrNegatedBool(Value)),
hasFalseExpression(literalOrNegatedBool(!Value)))
- .bind(TernaryId),
+ .bind(Id),
this);
}
@@ -499,17 +508,66 @@ void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
this);
}
+static internal::Matcher<Stmt> ifReturnValue(bool Value) {
+ return ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))
+ .bind(CompoundIfId);
+}
+
+static internal::Matcher<Stmt> returnNotValue(bool Value) {
+ return returnStmt(has(literalOrNegatedBool(!Value))).bind(CompoundReturnId);
+}
+
void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
bool Value,
StringRef Id) {
- Finder->addMatcher(
- compoundStmt(
- hasAnySubstatement(
- ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
- hasAnySubstatement(returnStmt(has(literalOrNegatedBool(!Value)))
- .bind(CompoundReturnId)))
- .bind(Id),
- this);
+ if (ChainedConditionalReturn)
+ Finder->addMatcher(
+ compoundStmt(hasSubstatementSequence(ifReturnValue(Value),
+ returnNotValue(Value)))
+ .bind(Id),
+ this);
+ else
+ Finder->addMatcher(
+ compoundStmt(hasSubstatementSequence(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())),
+ unless(hasParent(ifStmt())))
+ .bind(CompoundIfId),
+ returnNotValue(Value)))
+ .bind(Id),
+ this);
+}
+
+void SimplifyBooleanExprCheck::matchCaseIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> CaseStmt =
+ caseStmt(hasSubstatement(ifReturnValue(Value))).bind(CaseId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(CaseStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
+}
+
+void SimplifyBooleanExprCheck::matchDefaultIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> DefaultStmt =
+ defaultStmt(hasSubstatement(ifReturnValue(Value))).bind(DefaultId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(DefaultStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
+}
+
+void SimplifyBooleanExprCheck::matchLabelIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> LabelStmt =
+ labelStmt(hasSubstatement(ifReturnValue(Value))).bind(LabelId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(LabelStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(CompoundStmt, this);
}
void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
@@ -535,6 +593,15 @@ void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
+
+ matchCaseIfReturnsBool(Finder, true, CaseCompoundBoolId);
+ matchCaseIfReturnsBool(Finder, false, CaseCompoundNotBoolId);
+
+ matchDefaultIfReturnsBool(Finder, true, DefaultCompoundBoolId);
+ matchDefaultIfReturnsBool(Finder, false, DefaultCompoundNotBoolId);
+
+ matchLabelIfReturnsBool(Finder, true, LabelCompoundBoolId);
+ matchLabelIfReturnsBool(Finder, false, LabelCompoundNotBoolId);
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
@@ -548,33 +615,48 @@ void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
replaceWithElseStatement(Result, FalseConditionRemoved);
else if (const auto *Ternary =
Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
- replaceWithCondition(Result, Ternary);
+ replaceWithCondition(Result, Ternary, false);
else if (const auto *TernaryNegated =
Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
replaceWithCondition(Result, TernaryNegated, true);
else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
- replaceWithReturnCondition(Result, If);
+ replaceWithReturnCondition(Result, If, false);
else if (const auto *IfNot =
Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
replaceWithReturnCondition(Result, IfNot, true);
else if (const auto *IfAssign =
Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
- replaceWithAssignment(Result, IfAssign);
+ replaceWithAssignment(Result, IfAssign, false);
else if (const auto *IfAssignNot =
Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId))
replaceWithAssignment(Result, IfAssignNot, true);
else if (const auto *Compound =
Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId))
- replaceCompoundReturnWithCondition(Result, Compound);
- else if (const auto *Compound =
+ replaceCompoundReturnWithCondition(Result, Compound, false);
+ else if (const auto *CompoundNot =
Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
- replaceCompoundReturnWithCondition(Result, Compound, true);
-}
-
-void SimplifyBooleanExprCheck::issueDiag(
- const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc,
- StringRef Description, SourceRange ReplacementRange,
- StringRef Replacement) {
+ replaceCompoundReturnWithCondition(Result, CompoundNot, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundNotBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundNotBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundBoolId))
+ replaceLabelCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundNotBoolId))
+ replaceLabelCompoundReturnWithCondition(Result, true);
+ else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
+ Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
+}
+
+void SimplifyBooleanExprCheck::issueDiag(const MatchFinder::MatchResult &Result,
+ SourceLocation Loc,
+ StringRef Description,
+ SourceRange ReplacementRange,
+ StringRef Replacement) {
CharSourceRange CharRange =
Lexer::makeFileCharRange(CharSourceRange::getTokenRange(ReplacementRange),
*Result.SourceManager, getLangOpts());
@@ -585,19 +667,19 @@ void SimplifyBooleanExprCheck::issueDiag(
}
void SimplifyBooleanExprCheck::replaceWithThenStatement(
- const MatchFinder::MatchResult &Result, const Expr *TrueConditionRemoved) {
+ const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
- issueDiag(Result, TrueConditionRemoved->getBeginLoc(),
- SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+ IfStatement->getSourceRange(),
getText(Result, *IfStatement->getThen()));
}
void SimplifyBooleanExprCheck::replaceWithElseStatement(
- const MatchFinder::MatchResult &Result, const Expr *FalseConditionRemoved) {
+ const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
const Stmt *ElseStatement = IfStatement->getElse();
- issueDiag(Result, FalseConditionRemoved->getBeginLoc(),
- SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+ IfStatement->getSourceRange(),
ElseStatement ? getText(Result, *ElseStatement) : "");
}
@@ -627,13 +709,7 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
bool Negated) {
const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
- // The body shouldn't be empty because the matcher ensures that it must
- // contain at least two statements:
- // 1) A `return` statement returning a boolean literal `false` or `true`
- // 2) An `if` statement with no `else` clause that consists of a single
- // `return` statement returning the opposite boolean literal `true` or
- // `false`.
- assert(Compound->size() >= 2);
+ // Scan through the CompoundStmt to look for a chained-if construct.
const IfStmt *BeforeIf = nullptr;
CompoundStmt::const_body_iterator Current = Compound->body_begin();
CompoundStmt::const_body_iterator After = Compound->body_begin();
@@ -645,9 +721,8 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
if (!ChainedConditionalReturn && BeforeIf)
continue;
- const Expr *Condition = If->getCond();
std::string Replacement =
- "return " + replacementExpression(Result, Negated, Condition);
+ "return " + replacementExpression(Result, Negated, If->getCond());
issueDiag(
Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
@@ -662,6 +737,38 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
}
}
+void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated, const IfStmt *If) {
+ const auto *Lit = stmtReturnsBool(If, Negated);
+ const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
+ const std::string Replacement =
+ "return " + replacementExpression(Result, Negated, If->getCond());
+ issueDiag(Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+}
+
+void SimplifyBooleanExprCheck::replaceCaseCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const auto *CaseDefault = Result.Nodes.getNodeAs<CaseStmt>(CaseId);
+ const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
+void SimplifyBooleanExprCheck::replaceDefaultCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const SwitchCase *CaseDefault =
+ Result.Nodes.getNodeAs<DefaultStmt>(DefaultId);
+ const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
+void SimplifyBooleanExprCheck::replaceLabelCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const auto *Label = Result.Nodes.getNodeAs<LabelStmt>(LabelId);
+ const auto *If = dyn_cast<IfStmt>(Label->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
void SimplifyBooleanExprCheck::replaceWithAssignment(
const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
bool Negated) {
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
index 626108cbe22c9..c5b4617a88e86 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -41,7 +41,7 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
StringRef BooleanId);
void matchTernaryResult(ast_matchers::MatchFinder *Finder, bool Value,
- StringRef TernaryId);
+ StringRef Id);
void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
StringRef Id);
@@ -52,30 +52,51 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
StringRef Id);
+ void matchCaseIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
+ void matchDefaultIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
+ void matchLabelIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
void
replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
const Expr *BoolLiteral);
void
replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult &Result,
- const Expr *FalseConditionRemoved);
+ const Expr *BoolLiteral);
void
replaceWithCondition(const ast_matchers::MatchFinder::MatchResult &Result,
- const ConditionalOperator *Ternary,
- bool Negated = false);
+ const ConditionalOperator *Ternary, bool Negated);
void replaceWithReturnCondition(
const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,
- bool Negated = false);
+ bool Negated);
void
replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
- const IfStmt *If, bool Negated = false);
+ const IfStmt *If, bool Negated);
void replaceCompoundReturnWithCondition(
const ast_matchers::MatchFinder::MatchResult &Result,
- const CompoundStmt *Compound, bool Negated = false);
+ const CompoundStmt *Compound, bool Negated);
+
+ void replaceCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated,
+ const IfStmt *If);
+
+ void replaceCaseCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
+
+ void replaceDefaultCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
+
+ void replaceLabelCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result,
SourceLocation Loc, StringRef Description,
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
new file mode 100644
index 0000000000000..62fc9cfcaa1d3
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
@@ -0,0 +1,67 @@
+//===-- SimplifyBooleanExprMatchers.h - clang-tidy ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+
+namespace clang {
+namespace ast_matchers {
+
+/// Matches the substatement associated with a case, default or label statement.
+///
+/// Given
+/// \code
+/// switch (1) { case 1: break; case 2: return; break; default: return; break;
+/// }
+/// foo: return;
+/// bar: break;
+/// \endcode
+///
+/// caseStmt(hasSubstatement(returnStmt()))
+/// matches "case 2: return;"
+/// defaultStmt(hasSubstatement(returnStmt()))
+/// matches "default: return;"
+/// labelStmt(hasSubstatement(breakStmt()))
+/// matches "bar: break;"
+AST_POLYMORPHIC_MATCHER_P(hasSubstatement,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(CaseStmt, DefaultStmt,
+ LabelStmt),
+ internal::Matcher<Stmt>, InnerMatcher) {
+ return InnerMatcher.matches(*Node.getSubStmt(), Finder, Builder);
+}
+
+/// Matches two consecutive statements within a compound statement.
+///
+/// Given
+/// \code
+/// { if (x > 0) return true; return false; }
+/// \endcode
+/// compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))
+/// matches '{ if (x > 0) return true; return false; }'
+AST_POLYMORPHIC_MATCHER_P2(hasSubstatementSequence,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(CompoundStmt,
+ StmtExpr),
+ internal::Matcher<Stmt>, InnerMatcher1,
+ internal::Matcher<Stmt>, InnerMatcher2) {
+ if (const CompoundStmt *CS = CompoundStmtMatcher<NodeType>::get(Node)) {
+ auto It = matchesFirstInPointerRange(InnerMatcher1, CS->body_begin(),
+ CS->body_end(), Finder, Builder);
+ while (It != CS->body_end()) {
+ ++It;
+ if (It == CS->body_end())
+ return false;
+ if (InnerMatcher2.matches(**It, Finder, Builder))
+ return true;
+ It = matchesFirstInPointerRange(InnerMatcher1, It, CS->body_end(), Finder,
+ Builder);
+ }
+ }
+ return false;
+}
+
+} // namespace ast_matchers
+} // namespace clang
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp
new file mode 100644
index 0000000000000..2b3bf2e46d4c2
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,744 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+
+ case 1:
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 2:
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+
+ case 3:
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 4:
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+
+ case 5:
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 6:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+
+ case 7:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+
+ case 8:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+
+ case 9:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+
+ case 10:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+
+ case 11:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+
+ case 12:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+
+ case 13:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+
+ case 14:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+
+ case 15:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+
+ case 16:
+ if (j > 10)
+ return true;
+ return true;
+ return false;
+
+ case 17:
+ if (j > 10)
+ return false;
+ return false;
+ return true;
+
+ case 100: {
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+
+ case 101: {
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 102: {
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+
+ case 103: {
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 104: {
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+
+ case 105: {
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 106: {
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+ }
+
+ case 107: {
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+ }
+
+ case 108: {
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+ }
+
+ case 109: {
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+
+ case 110: {
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+
+ case 111: {
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+
+ case 112: {
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+
+ case 113: {
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+
+ case 114: {
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+
+ case 115: {
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+
+ case 116: {
+ return false;
+ if (j > 10)
+ return true;
+ }
+
+ case 117: {
+ return true;
+ if (j > 10)
+ return false;
+ }
+ }
+
+ return j > 0;
+}
+
+bool default_stmt0(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+ return false;
+}
+
+bool default_stmt1(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt2(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+ return false;
+}
+
+bool default_stmt3(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt4(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+ return false;
+}
+
+bool default_stmt5(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt6(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+ }
+ return false;
+}
+
+bool default_stmt7(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+ }
+ return false;
+}
+
+bool default_stmt8(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+ }
+ return false;
+}
+
+bool default_stmt9(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+ return false;
+}
+
+bool default_stmt10(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+ return false;
+}
+
+bool default_stmt11(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+ return false;
+}
+
+bool default_stmt12(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+ return false;
+}
+
+bool default_stmt13(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+ return false;
+}
+
+bool default_stmt14(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+ }
+ return false;
+}
+
+bool default_stmt15(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+ }
+ return false;
+}
+
+bool default_stmt16(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return false;
+
+ default:
+ if (j > 10)
+ return true;
+ }
+ return false;
+}
+
+bool default_stmt17(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ }
+ return false;
+}
+
+bool label_stmt0(int i, int j, bool b) {
+label:
+ if (b == true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt1(int i, int j, bool b) {
+label:
+ if (b == false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt2(int i, int j, bool b) {
+label:
+ if (b && true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt3(int i, int j, bool b) {
+label:
+ if (b && false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt4(int i, int j, bool b) {
+label:
+ if (b || true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt5(int i, int j, bool b) {
+label:
+ if (b || false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt6(int i, int j, bool b) {
+label:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+}
+
+bool label_stmt7(int i, int j, bool b) {
+label:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+}
+
+bool label_stmt8(int i, int j, bool b) {
+label:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ return false;
+}
+
+bool label_stmt9(int i, int j, bool b) {
+label:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt10(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+}
+
+bool label_stmt11(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+}
+
+bool label_stmt12(int i, int j, bool b) {
+label:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+}
+
+bool label_stmt13(int i, int j, bool b) {
+label:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+}
+
+bool label_stmt14(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+}
+
+bool label_stmt15(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
index a00b733ec1eae..acf0cfe301279 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
@@ -581,19 +581,19 @@ void complex_conditional_assignment_statements(int i) {
// Unchanged: multiple statements.
bool g;
if (j > 10)
- f = true;
+ g = true;
else {
j = 20;
- f = false;
+ g = false;
}
// Unchanged: multiple statements.
bool h;
if (j > 10) {
j = 10;
- f = true;
+ h = true;
} else
- f = false;
+ h = false;
}
// Unchanged: chained return statements, but ChainedConditionalReturn not set.
diff --git a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt b/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
index 6c9fa61cf92ea..253f78eb36ea9 100644
--- a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -42,6 +42,7 @@ clang_target_link_libraries(ClangTidyTests
clangFrontend
clangLex
clangSerialization
+ clangTesting
clangTooling
clangToolingCore
clangTransformer
diff --git a/clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp b/clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
index a0eca16265101..f6a28d92874ed 100644
--- a/clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
@@ -1,6 +1,9 @@
+#include "../../clang/unittests/ASTMatchers/ASTMatchersTest.h"
#include "ClangTidyTest.h"
#include "readability/BracesAroundStatementsCheck.h"
#include "readability/NamespaceCommentCheck.h"
+#include "readability/SimplifyBooleanExprMatchers.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "gtest/gtest.h"
namespace clang {
@@ -9,6 +12,123 @@ namespace test {
using readability::BracesAroundStatementsCheck;
using readability::NamespaceCommentCheck;
+using namespace ast_matchers;
+
+TEST_P(ASTMatchersTest, HasCaseSubstatement) {
+ EXPECT_TRUE(matches(
+ "void f() { switch (1) { case 1: return; break; default: break; } }",
+ traverse(TK_AsIs, caseStmt(hasSubstatement(returnStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasDefaultSubstatement) {
+ EXPECT_TRUE(matches(
+ "void f() { switch (1) { case 1: return; break; default: break; } }",
+ traverse(TK_AsIs, defaultStmt(hasSubstatement(breakStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasLabelSubstatement) {
+ EXPECT_TRUE(
+ matches("void f() { while (1) { bar: break; foo: return; } }",
+ traverse(TK_AsIs, labelStmt(hasSubstatement(breakStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceSimple) {
+ const char *Text = "int f() { int x = 5; if (x < 0) return 1; return 0; }";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), labelStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(returnStmt(), ifStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(switchStmt(), labelStmt()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceAlmost) {
+ const char *Text = R"code(
+int f() {
+ int x = 5;
+ if (x < 10)
+ ;
+ if (x < 0)
+ return 1;
+ return 0;
+}
+)code";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_TRUE(
+ matches(Text, compoundStmt(hasSubstatementSequence(ifStmt(), ifStmt()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceComplex) {
+ const char *Text = R"code(
+int f() {
+ int x = 5;
+ if (x < 10)
+ x -= 10;
+ if (x < 0)
+ return 1;
+ return 0;
+}
+)code";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_FALSE(
+ matches(Text, compoundStmt(hasSubstatementSequence(ifStmt(), expr()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceExpression) {
+ const char *Text = R"code(
+int f() {
+ return ({ int x = 5;
+ int result;
+ if (x < 10)
+ x -= 10;
+ if (x < 0)
+ result = 1;
+ else
+ result = 0;
+ result;
+ });
+ }
+)code";
+ EXPECT_TRUE(
+ matches(Text, stmtExpr(hasSubstatementSequence(ifStmt(), expr()))));
+ EXPECT_FALSE(
+ matches(Text, stmtExpr(hasSubstatementSequence(ifStmt(), returnStmt()))));
+}
+
+// Copied from ASTMatchersTests
+static std::vector<TestClangConfig> allTestClangConfigs() {
+ std::vector<TestClangConfig> all_configs;
+ for (TestLanguage lang : {Lang_C89, Lang_C99, Lang_CXX03, Lang_CXX11,
+ Lang_CXX14, Lang_CXX17, Lang_CXX20}) {
+ TestClangConfig config;
+ config.Language = lang;
+
+ // Use an unknown-unknown triple so we don't instantiate the full system
+ // toolchain. On Linux, instantiating the toolchain involves stat'ing
+ // large portions of /usr/lib, and this slows down not only this test, but
+ // all other tests, via contention in the kernel.
+ //
+ // FIXME: This is a hack to work around the fact that there's no way to do
+ // the equivalent of runToolOnCodeWithArgs without instantiating a full
+ // Driver. We should consider having a function, at least for tests, that
+ // invokes cc1.
+ config.Target = "i386-unknown-unknown";
+ all_configs.push_back(config);
+
+ // Windows target is interesting to test because it enables
+ // `-fdelayed-template-parsing`.
+ config.Target = "x86_64-pc-win32-msvc";
+ all_configs.push_back(config);
+ }
+ return all_configs;
+}
+
+INSTANTIATE_TEST_SUITE_P(ASTMatchersTests, ASTMatchersTest,
+ testing::ValuesIn(allTestClangConfigs()));
TEST(NamespaceCommentCheckTest, Basic) {
EXPECT_EQ("namespace i {\n} // namespace i",
@@ -488,9 +608,9 @@ TEST(BracesAroundStatementsCheckTest, ImplicitCastInReturn) {
Opts.CheckOptions["test-check-0.ShortStatementLines"] = "1";
StringRef Input = "const char *f() {\n"
- " if (true) return \"\";\n"
- " return \"abc\";\n"
- "}\n";
+ " if (true) return \"\";\n"
+ " return \"abc\";\n"
+ "}\n";
EXPECT_NO_CHANGES_WITH_OPTS(BracesAroundStatementsCheck, Opts, Input);
EXPECT_EQ("const char *f() {\n"
" if (true) { return \"\";\n"
More information about the cfe-commits
mailing list