LegalizeAdulthood updated this revision to Diff 180385.
LegalizeAdulthood added a comment.
No really, update from review comments `:)`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56303/new/
https://reviews.llvm.org/D56303
Files:
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tidy/readability/SimplifyBooleanExprCheck.h
test/clang-tidy/readability-simplify-bool-expr-case.cpp
Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// 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;
+}
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -50,6 +50,12 @@
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
replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
const CXXBoolLiteralExpr *BoolLiteral);
@@ -60,20 +66,31 @@
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 replaceSwitchCaseCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const CompoundStmt *Compound, bool Negated, const SwitchCase *SwitchCase);
+
+ void replaceCaseCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const CompoundStmt *Compound, bool Negated);
+
+ void replaceDefaultCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const CompoundStmt *Compound, bool Negated);
void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result,
SourceLocation Loc, StringRef Description,
Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -47,8 +47,15 @@
const char IfAssignNotBoolId[] = "if-assign-not";
const char IfAssignObjId[] = "if-assign-obj";
const char CompoundReturnId[] = "compound-return";
+const char CompoundIfId[] = "compound-if";
const char CompoundBoolId[] = "compound-bool";
const char CompoundNotBoolId[] = "compound-bool-not";
+const char CaseId[] = "case";
+const char CaseCompoundBoolId[] = "case-compound-bool";
+const char CaseCompoundNotBoolId[] = "case-compound-bool-not";
+const char DefaultId[] = "default";
+const char DefaultCompoundBoolId[] = "default-compound-bool";
+const char DefaultCompoundNotBoolId[] = "default-compound-bool-not";
const char IfStmtId[] = "if";
@@ -319,7 +326,7 @@
} // namespace
class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
- public:
+public:
Visitor(SimplifyBooleanExprCheck *Check,
const MatchFinder::MatchResult &Result)
: Check(Check), Result(Result) {}
@@ -329,12 +336,11 @@
return true;
}
- private:
+private:
SimplifyBooleanExprCheck *Check;
const MatchFinder::MatchResult &Result;
};
-
SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -361,8 +367,8 @@
const auto *LHS = Op->getLHS()->IgnoreParenImpCasts();
const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
- const CXXBoolLiteralExpr *Bool = nullptr;
- const Expr *Other = nullptr;
+ const CXXBoolLiteralExpr *Bool;
+ const Expr *Other;
if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)))
Other = RHS;
else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)))
@@ -377,46 +383,46 @@
if (!isa<CXXBoolLiteralExpr>(Other) && containsBoolLiteral(Other))
return;
- bool BoolValue = Bool->getValue();
+ const bool BoolValue = Bool->getValue();
- auto replaceWithExpression = [this, &Result, LHS, RHS, Bool](
- const Expr *ReplaceWith, bool Negated) {
- std::string Replacement =
- replacementExpression(Result, Negated, ReplaceWith);
- SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc());
- issueDiag(Result, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range,
- Replacement);
- };
+ const auto replaceWithExpression =
+ [this, &Result, LHS, RHS, Bool](const Expr *ReplaceWith, bool Negated) {
+ const std::string Replacement =
+ replacementExpression(Result, Negated, ReplaceWith);
+ SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc());
+ issueDiag(Result, Bool->getBeginLoc(), SimplifyOperatorDiagnostic,
+ Range, Replacement);
+ };
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;
}
}
@@ -499,6 +505,42 @@
this);
}
+void SimplifyBooleanExprCheck::matchCaseIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ // Binding to the returnStmt matched is pointless because we can't guarantee
+ // anything about the ordering of the return statement and the case statement.
+ // If only we had a statement sequence matcher...
+ Finder->addMatcher(
+ switchStmt(
+ has(compoundStmt(
+ has(caseStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())))
+ .bind(CompoundIfId)))
+ .bind(CaseId)),
+ has(returnStmt(has(cxxBoolLiteral(equals(!Value))))))
+ .bind(Id))),
+ this);
+}
+
+void SimplifyBooleanExprCheck::matchDefaultIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ // Binding to the returnStmt matched is pointless because we can't guarantee
+ // anything about the ordering of the return statement and the default statement.
+ // If only we had a statement sequence matcher...
+ Finder->addMatcher(
+ switchStmt(has(
+ compoundStmt(
+ has(defaultStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())))
+ .bind(CompoundIfId)))
+ .bind(DefaultId)),
+ has(returnStmt(has(cxxBoolLiteral(equals(!Value))))))
+ .bind(Id))),
+ this);
+}
+
void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
Options.store(Opts, "ChainedConditionalAssignment",
@@ -522,6 +564,12 @@
matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
+
+ matchCaseIfReturnsBool(Finder, true, CaseCompoundBoolId);
+ matchCaseIfReturnsBool(Finder, false, CaseCompoundNotBoolId);
+
+ matchDefaultIfReturnsBool(Finder, true, DefaultCompoundBoolId);
+ matchDefaultIfReturnsBool(Finder, false, DefaultCompoundNotBoolId);
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
@@ -535,27 +583,41 @@
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);
+ replaceCompoundReturnWithCondition(Result, CompoundNot, true);
+ else if (const auto *CaseCompound =
+ Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, CaseCompound, false);
+ else if (const auto *CaseCompoundNot =
+ Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundNotBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, CaseCompoundNot, true);
+ else if (const auto *DefaultCompound =
+ Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, DefaultCompound, false);
+ else if (const auto *DefaultCompoundNot =
+ Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundNotBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, DefaultCompoundNot, true);
+ else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
+ Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
}
void SimplifyBooleanExprCheck::issueDiag(
@@ -622,7 +684,7 @@
// 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);
+ assert((Compound->size() >= 2) && "CompoundStmt must contain at least 2 statements; matcher error?");
const IfStmt *BeforeIf = nullptr;
CompoundStmt::const_body_iterator Current = Compound->body_begin();
CompoundStmt::const_body_iterator After = Compound->body_begin();
@@ -635,7 +697,7 @@
continue;
const Expr *Condition = If->getCond();
- std::string Replacement =
+ const std::string Replacement =
"return " + replacementExpression(Result, Negated, Condition);
issueDiag(
Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
@@ -651,6 +713,87 @@
}
}
+void SimplifyBooleanExprCheck::replaceSwitchCaseCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+ bool Negated, const SwitchCase *CaseDefault) {
+ assert((CaseDefault != nullptr) && "null SwitchCase pointer; matcher error?");
+
+ // 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) && "CompoundStmt must contain at least 2 statements; matcher error?");
+
+ // We're looking for something like:
+ //
+ // switch (expr) {
+ // case 10:
+ // if (cond) return false;
+ // return true;
+ // }
+ //
+ // The matcher ensures that the case/if combination is already there,
+ // and that there is a return statement of this form somewhere in the
+ // compound statement associated with the switch contains a return
+ // statement of this form, but the return statement could be anywhere
+ // within the compound statement, so we have to manually check to see
+ // if the next statement after the if contains a return statement of
+ // the correct form. Ideally we'd be able to express this with the
+ // matchers, but that is currently impossible.
+ //
+ const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+ assert((If != nullptr) && "Expected an IfStmt inside SwitchCase; matcher error?");
+ const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated);
+ assert((Lit != nullptr) && "Expected ReturnStmt returning bool inside SwitchCase; matcher error?");
+
+ // Advance to the matched CaseStmt
+ CompoundStmt::const_body_iterator Current = Compound->body_begin();
+ while (Current != Compound->body_end() && *Current != CaseDefault)
+ ++Current;
+ assert((Current != Compound->body_end()) && "Expected additional statement inside SwitchCase; matcher error?");
+
+ // Advance to the next statement, watch out for something like:
+ //
+ // switch (expr) {
+ // case 0:
+ // return true;
+ //
+ // case 1:
+ // if (cond) return false;
+ // }
+ //
+ ++Current;
+ if (Current == Compound->body_end())
+ return;
+
+ if (const auto *Ret = dyn_cast<ReturnStmt>(*Current)) {
+ if (stmtReturnsBool(Ret, !Negated)) {
+ const Expr *Condition = If->getCond();
+ const std::string Replacement =
+ "return " + replacementExpression(Result, Negated, Condition);
+ issueDiag(Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+ }
+ }
+}
+
+void SimplifyBooleanExprCheck::replaceCaseCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+ bool Negated) {
+ replaceSwitchCaseCompoundReturnWithCondition(
+ Result, Compound, Negated, Result.Nodes.getNodeAs<CaseStmt>(CaseId));
+}
+
+void SimplifyBooleanExprCheck::replaceDefaultCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+ bool Negated) {
+ replaceSwitchCaseCompoundReturnWithCondition(
+ Result, Compound, Negated,
+ Result.Nodes.getNodeAs<DefaultStmt>(DefaultId));
+}
+
void SimplifyBooleanExprCheck::replaceWithAssignment(
const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
bool Negated) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
