Update from review comments and handle chained condition returns of the form
if (expr)
return true;
if (expr2)
return true;
return false;
and variants.
http://reviews.llvm.org/D9810
Files:
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tidy/readability/SimplifyBooleanExprCheck.h
test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
test/clang-tidy/readability-simplify-bool-expr.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -10,7 +10,6 @@
#include "SimplifyBooleanExprCheck.h"
#include "clang/Lex/Lexer.h"
-#include <cassert>
#include <string>
using namespace clang::ast_matchers;
@@ -48,6 +47,11 @@
const char IfAssignBoolId[] = "if-assign";
const char IfAssignNotBoolId[] = "if-assign-not";
const char IfAssignObjId[] = "if-assign-obj";
+const char CompoundId[] = "compound";
+const char CompoundIfReturnsBoolId[] = "compound-if";
+const char CompoundReturnId[] = "compound-return";
+const char CompoundBoolId[] = "compound-bool";
+const char CompoundNotBoolId[] = "compound-bool-not";
const char IfStmtId[] = "if";
const char LHSId[] = "lhs-expr";
@@ -57,6 +61,8 @@
"redundant boolean literal supplied to boolean operator";
const char SimplifyConditionDiagnostic[] =
"redundant boolean literal in if statement condition";
+const char SimplifyConditionalReturnDiagnostic[] =
+ "redundant boolean literal in conditional return statement";
const CXXBoolLiteralExpr *getBoolLiteral(const MatchFinder::MatchResult &Result,
StringRef Id) {
@@ -67,9 +73,9 @@
: Literal;
}
-internal::Matcher<Stmt> ReturnsBool(bool Value, StringRef Id = "") {
- auto SimpleReturnsBool = returnStmt(
- has(boolLiteral(equals(Value)).bind(Id.empty() ? "ignored" : Id)));
+internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
+ auto SimpleReturnsBool =
+ returnStmt(has(boolLiteral(equals(Value)).bind(Id))).bind("returns-bool");
return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
}
@@ -110,14 +116,47 @@
return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
" " + getText(Result, *BinOp->getRHS())).str();
}
+ } else if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
+ if (UnOp->getOpcode() == UO_LNot) {
+ return replacementExpression(Result, false, UnOp->getSubExpr());
+ }
}
}
StringRef Text = getText(Result, *E);
return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
: "!" + Text)
: Text).str();
}
+const CXXBoolLiteralExpr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+ if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
+ if (Bool->getValue() == !Negated) {
+ return Bool;
+ }
+ }
+
+ return nullptr;
+}
+
+const CXXBoolLiteralExpr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
+ if (IfRet->getElse() != nullptr) {
+ return nullptr;
+ }
+
+ if (const auto *Ret = dyn_cast<ReturnStmt>(IfRet->getThen())) {
+ return stmtReturnsBool(Ret, Negated);
+ } else if (const auto *Compound = dyn_cast<CompoundStmt>(IfRet->getThen())) {
+ if (Compound->size() == 1) {
+ if (const auto *CompoundRet =
+ dyn_cast<ReturnStmt>(Compound->body_back())) {
+ return stmtReturnsBool(CompoundRet, Negated);
+ }
+ }
+ }
+
+ return nullptr;
+}
+
} // namespace
SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
@@ -206,14 +245,14 @@
bool Value, StringRef Id) {
if (ChainedConditionalReturn) {
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
- hasThen(ReturnsBool(Value, ThenLiteralId)),
- hasElse(ReturnsBool(!Value))).bind(Id),
+ hasThen(returnsBool(Value, ThenLiteralId)),
+ hasElse(returnsBool(!Value))).bind(Id),
this);
} else {
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())),
- hasThen(ReturnsBool(Value, ThenLiteralId)),
- hasElse(ReturnsBool(!Value))).bind(Id),
+ hasThen(returnsBool(Value, ThenLiteralId)),
+ hasElse(returnsBool(!Value))).bind(Id),
this);
}
}
@@ -245,6 +284,18 @@
}
}
+void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ Finder->addMatcher(
+ compoundStmt(
+ allOf(hasAnySubstatement(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())))),
+ hasAnySubstatement(returnStmt(has(boolLiteral(equals(!Value))))
+ .bind(CompoundReturnId)))).bind(Id),
+ this);
+}
+
void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
Options.store(Opts, "ChainedConditionalAssignment",
@@ -283,6 +334,9 @@
matchIfAssignsBool(Finder, true, IfAssignBoolId);
matchIfAssignsBool(Finder, false, IfAssignNotBoolId);
+
+ matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
+ matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
@@ -322,6 +376,12 @@
} 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 =
+ Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId)) {
+ replaceCompoundReturnWithCondition(Result, Compound, true);
}
}
@@ -375,10 +435,45 @@
std::string Replacement = ("return " + Condition + Terminator).str();
SourceLocation Start =
Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
- diag(Start, "redundant boolean literal in conditional return statement")
+ diag(Start, SimplifyConditionalReturnDiagnostic)
<< FixItHint::CreateReplacement(If->getSourceRange(), Replacement);
}
+void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
+ bool Negated) {
+ const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
+
+ const IfStmt *BeforeIf = nullptr;
+ CompoundStmt::const_body_iterator Current = Compound->body_begin();
+ CompoundStmt::const_body_iterator After = Compound->body_begin();
+ for (++After; After != Compound->body_end() && *Current != Ret;
+ ++Current, ++After) {
+ if (const auto *If = dyn_cast<IfStmt>(*Current)) {
+ if (const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated)) {
+ if (*After == Ret) {
+ if (!ChainedConditionalReturn && BeforeIf) {
+ continue;
+ }
+
+ const Expr *Condition = If->getCond();
+ std::string Replacement =
+ "return " + replacementExpression(Result, Negated, Condition);
+ diag(Lit->getLocStart(), SimplifyConditionalReturnDiagnostic)
+ << FixItHint::CreateReplacement(
+ SourceRange(If->getLocStart(), Ret->getLocEnd()),
+ Replacement);
+ return;
+ }
+
+ BeforeIf = If;
+ }
+ } else {
+ BeforeIf = nullptr;
+ }
+ }
+}
+
void SimplifyBooleanExprCheck::replaceWithAssignment(
const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
bool Negated) {
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -34,6 +34,8 @@
/// `if (e) return false; else return true;` becomes `return !e;`
/// `if (e) b = true; else b = false;` becomes `b = e;`
/// `if (e) b = false; else b = true;` becomes `b = !e;`
+/// `if (e) return true; return false;` becomes `return e;`
+/// `if (e) return false; return true;` becomes `return !e;`
///
/// Parenthesis from the resulting expression `e` are removed whenever
/// possible.
@@ -77,6 +79,9 @@
void matchIfAssignsBool(ast_matchers::MatchFinder *Finder, bool Value,
StringRef Id);
+ void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
void
replaceWithExpression(const ast_matchers::MatchFinder::MatchResult &Result,
const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS,
@@ -103,6 +108,10 @@
replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
const IfStmt *If, bool Negated = false);
+ void replaceCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ const CompoundStmt *Compound, bool Negated = false);
+
const bool ChainedConditionalReturn;
const bool ChainedConditionalAssignment;
};
Index: test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
===================================================================
--- test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
+++ test/clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
@@ -31,3 +31,65 @@
// CHECK-FIXES-NEXT: {{^}} return false;
// CHECK-FIXES-NEXT: {{^}} else return i > 20;
}
+
+bool chained_simple_if_return(int i) {
+ if (i < 5)
+ return true;
+ if (i > 10)
+ return true;
+ return false;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool chained_simple_if_return(int i) {{{$}}
+// CHECK-FIXES: {{^}} if (i < 5){{$}}
+// CHECK-FIXES: {{^ return true;$}}
+// CHECK-FIXES: {{^ return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool chained_simple_if_return_negated(int i) {
+ if (i < 5)
+ return false;
+ if (i > 10)
+ return false;
+ return true;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool chained_simple_if_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^}} if (i < 5){{$}}
+// CHECK-FIXES: {{^ return false;$}}
+// CHECK-FIXES: {{^ return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_chained_if_return_return(int i) {
+ if (i < 5) {
+ return true;
+ }
+ if (i > 10) {
+ return true;
+ }
+ return false;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_chained_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^}} if (i < 5) {{{$}}
+// CHECK-FIXES: {{^}} return true;{{$}}
+// CHECK-FIXES: {{^}} }{{$}}
+// CHECK-FIXES: {{^ return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_chained_if_return_return_negated(int i) {
+ if (i < 5) {
+ return false;
+ }
+ if (i > 10) {
+ return false;
+ }
+ return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_chained_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^}} if (i < 5) {{{$}}
+// CHECK-FIXES: {{^}} return false;{{$}}
+// CHECK-FIXES: {{^}} }{{$}}
+// CHECK-FIXES: {{^ return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===================================================================
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -321,6 +321,13 @@
// CHECK-FIXES: {{^}} return i != 0;{{$}}
// CHECK-FIXES-NEXT: {{^}$}}
+bool negative_condition_conditional_return_statement(int i) {
+ if (!(i == 0)) return false; else return true;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{^}} return i == 0;{{$}}
+// CHECK-FIXES-NEXT: {{^}$}}
+
bool conditional_compound_return_statements(int i) {
if (i == 1) {
return true;
@@ -593,3 +600,85 @@
else
b = false;
}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_simple_if_return_negated(int i) {
+ if (i < 5)
+ return false;
+ if (i > 10)
+ return false;
+ return true;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool complex_chained_if_return_return(int i) {
+ if (i < 5) {
+ return true;
+ }
+ if (i > 10) {
+ return true;
+ }
+ return false;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool complex_chained_if_return_return_negated(int i) {
+ if (i < 5) {
+ return false;
+ }
+ if (i > 10) {
+ return false;
+ }
+ return true;
+}
+
+// unchanged: chained return statements, but ChainedConditionalReturn not set
+bool chained_simple_if_return(int i) {
+ if (i < 5)
+ return true;
+ if (i > 10)
+ return true;
+ return false;
+}
+
+bool simple_if_return_return(int i) {
+ if (i > 10)
+ return true;
+ return false;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^ return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool simple_if_return_return_negated(int i) {
+ if (i > 10)
+ return false;
+ return true;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool simple_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^ return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_if_return_return(int i) {
+ if (i > 10) {
+ return true;
+ }
+ return false;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_if_return_return(int i) {{{$}}
+// CHECK-FIXES: {{^ return i > 10;$}}
+// CHECK-FIXES: {{^}$}}
+
+bool complex_if_return_return_negated(int i) {
+ if (i > 10) {
+ return false;
+ }
+ return true;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}bool complex_if_return_return_negated(int i) {{{$}}
+// CHECK-FIXES: {{^ return i <= 10;$}}
+// CHECK-FIXES: {{^}$}}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
