Update from comments
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
@@ -12,6 +12,7 @@
#include <cassert>
#include <string>
+#include <utility>
using namespace clang::ast_matchers;
@@ -48,6 +49,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 +63,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,14 +75,15 @@
: 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)));
}
bool needsParensAfterUnaryNegation(const Expr *E) {
+ E = E->IgnoreImpCasts();
if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
return true;
if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
@@ -84,8 +93,7 @@
}
std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
- std::make_pair(BO_LT, BO_GE), std::make_pair(BO_GT, BO_LE),
- std::make_pair(BO_EQ, BO_NE)};
+ {BO_LT, BO_GE}, {BO_GT, BO_LE}, {BO_EQ, BO_NE}};
StringRef negatedOperator(const BinaryOperator *BinOp) {
const BinaryOperatorKind Opcode = BinOp->getOpcode();
@@ -98,24 +106,145 @@
return StringRef();
}
+std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
+ {OO_EqualEqual, "=="},
+ {OO_ExclaimEqual, "!="},
+ {OO_Less, "<"},
+ {OO_GreaterEqual, ">="},
+ {OO_Greater, ">"},
+ {OO_LessEqual, "<="}};
+
+StringRef getOperatorName(OverloadedOperatorKind OpKind) {
+ for (auto Name : OperatorNames)
+ if (Name.first == OpKind)
+ return Name.second;
+
+ return StringRef();
+}
+
+std::pair<OverloadedOperatorKind, OverloadedOperatorKind> OppositeOverloads[] =
+ {{OO_EqualEqual, OO_ExclaimEqual},
+ {OO_Less, OO_GreaterEqual},
+ {OO_Greater, OO_LessEqual}};
+
+StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
+ const OverloadedOperatorKind Opcode = OpCall->getOperator();
+ for (auto NegatableOp : OppositeOverloads) {
+ if (Opcode == NegatableOp.first)
+ return getOperatorName(NegatableOp.second);
+ if (Opcode == NegatableOp.second)
+ return getOperatorName(NegatableOp.first);
+ }
+ return StringRef();
+}
+
+std::string asBool(StringRef text, bool NeedsStaticCast) {
+ if (NeedsStaticCast)
+ return ("static_cast<bool>(" + text + ")").str();
+
+ return text;
+}
+
+bool needsNullPtrComparison(const Expr *E) {
+ if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
+ return ImpCast->getCastKind() == CK_PointerToBoolean;
+
+ return false;
+}
+
+bool needsStaticCast(const Expr *E) {
+ if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
+ if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
+ ImpCast->getSubExpr()->getType()->isBooleanType())
+ if (const auto *MemCall =
+ dyn_cast<CXXMemberCallExpr>(ImpCast->getSubExpr()))
+ if (const auto *MemDecl =
+ dyn_cast<CXXConversionDecl>(MemCall->getMethodDecl()))
+ if (MemDecl->isExplicit())
+ return true;
+
+ E = E->IgnoreImpCasts();
+ return !E->getType()->isBooleanType();
+}
+
std::string replacementExpression(const MatchFinder::MatchResult &Result,
bool Negated, const Expr *E) {
- while (const auto *Parenthesized = dyn_cast<ParenExpr>(E)) {
- E = Parenthesized->getSubExpr();
- }
+ E = E->ignoreParenBaseCasts();
+ const bool NeedsStaticCast = needsStaticCast(E);
if (Negated) {
+ if (const auto *UnOp = dyn_cast<UnaryOperator>(E))
+ if (UnOp->getOpcode() == UO_LNot) {
+ if (needsNullPtrComparison(UnOp->getSubExpr()))
+ return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str();
+
+ return replacementExpression(Result, false, UnOp->getSubExpr());
+ }
+
+ if (needsNullPtrComparison(E))
+ return (getText(Result, *E) + " == nullptr").str();
+
+ StringRef NegatedOperator;
+ const Expr *LHS = nullptr;
+ const Expr *RHS = nullptr;
if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
- StringRef NegatedOperator = negatedOperator(BinOp);
- if (!NegatedOperator.empty()) {
- return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
- " " + getText(Result, *BinOp->getRHS())).str();
+ NegatedOperator = negatedOperator(BinOp);
+ LHS = BinOp->getLHS();
+ RHS = BinOp->getRHS();
+ } else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
+ if (OpExpr->getNumArgs() == 2) {
+ NegatedOperator = negatedOperator(OpExpr);
+ LHS = OpExpr->getArg(0);
+ RHS = OpExpr->getArg(1);
}
}
+ if (!NegatedOperator.empty() && LHS && RHS)
+ return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " +
+ getText(Result, *RHS)).str(),
+ NeedsStaticCast));
+
+ StringRef Text = getText(Result, *E);
+ if (!NeedsStaticCast && needsParensAfterUnaryNegation(E))
+ return ("!(" + Text + ")").str();
+
+ if (needsNullPtrComparison(E))
+ return (getText(Result, *E) + " == nullptr").str();
+
+ return ("!" + asBool(Text, NeedsStaticCast));
}
- StringRef Text = getText(Result, *E);
- return (Negated ? (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")"
- : "!" + Text)
- : Text).str();
+
+ if (const auto *UnOp = dyn_cast<UnaryOperator>(E))
+ if (UnOp->getOpcode() == UO_LNot)
+ if (needsNullPtrComparison(UnOp->getSubExpr()))
+ return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str();
+
+ if (needsNullPtrComparison(E))
+ return (getText(Result, *E) + " != nullptr").str();
+
+ return asBool(getText(Result, *E), NeedsStaticCast);
+}
+
+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);
+
+ 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
@@ -204,18 +333,17 @@
void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
bool Value, StringRef Id) {
- if (ChainedConditionalReturn) {
+ 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 {
+ 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);
- }
}
void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
@@ -233,16 +361,27 @@
hasRHS(boolLiteral(equals(!Value))));
auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
hasAnySubstatement(SimpleElse)));
- if (ChainedConditionalAssignment) {
+ if (ChainedConditionalAssignment)
Finder->addMatcher(
ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
this);
- } else {
+ else
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())), hasThen(Then),
hasElse(Else)).bind(Id),
this);
- }
+}
+
+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) {
@@ -283,46 +422,54 @@
matchIfAssignsBool(Finder, true, IfAssignBoolId);
matchIfAssignsBool(Finder, false, IfAssignNotBoolId);
+
+ matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
+ matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
if (const CXXBoolLiteralExpr *LeftRemoved =
- getBoolLiteral(Result, RightExpressionId)) {
+ getBoolLiteral(Result, RightExpressionId))
replaceWithExpression(Result, LeftRemoved, false);
- } else if (const CXXBoolLiteralExpr *RightRemoved =
- getBoolLiteral(Result, LeftExpressionId)) {
+ else if (const CXXBoolLiteralExpr *RightRemoved =
+ getBoolLiteral(Result, LeftExpressionId))
replaceWithExpression(Result, RightRemoved, true);
- } else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
- getBoolLiteral(Result, NegatedRightExpressionId)) {
+ else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
+ getBoolLiteral(Result, NegatedRightExpressionId))
replaceWithExpression(Result, NegatedLeftRemoved, false, true);
- } else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
- getBoolLiteral(Result, NegatedLeftExpressionId)) {
+ else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
+ getBoolLiteral(Result, NegatedLeftExpressionId))
replaceWithExpression(Result, NegatedRightRemoved, true, true);
- } else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
- getBoolLiteral(Result, ConditionThenStmtId)) {
+ else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
+ getBoolLiteral(Result, ConditionThenStmtId))
replaceWithThenStatement(Result, TrueConditionRemoved);
- } else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
- getBoolLiteral(Result, ConditionElseStmtId)) {
+ else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
+ getBoolLiteral(Result, ConditionElseStmtId))
replaceWithElseStatement(Result, FalseConditionRemoved);
- } else if (const auto *Ternary =
- Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId)) {
+ else if (const auto *Ternary =
+ Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
replaceWithCondition(Result, Ternary);
- } else if (const auto *TernaryNegated =
+ else if (const auto *TernaryNegated =
Result.Nodes.getNodeAs<ConditionalOperator>(
- TernaryNegatedId)) {
+ TernaryNegatedId))
replaceWithCondition(Result, TernaryNegated, true);
- } else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId)) {
+ else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
replaceWithReturnCondition(Result, If);
- } else if (const auto *IfNot =
- Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId)) {
+ else if (const auto *IfNot =
+ Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
replaceWithReturnCondition(Result, IfNot, true);
- } else if (const auto *IfAssign =
- Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId)) {
+ else if (const auto *IfAssign =
+ Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
replaceWithAssignment(Result, IfAssign);
- } else if (const auto *IfAssignNot =
- Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId)) {
+ 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);
}
void SimplifyBooleanExprCheck::replaceWithExpression(
@@ -375,10 +522,50 @@
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);
+
+ // 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 fo a single
+ // `return` statement returning the opposite boolean literal `true` or
+ // `false`.
+ assert(Compound->size() >= 2);
+ 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,9 +34,46 @@
/// `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.
+/// The resulting expression `e` is modified as follows:
+/// 1. Unnecessary parentheses around the expression are removed.
+/// 2. Negated applications of `!` are eliminated.
+/// 3. Negated applications of comparison operators are changed to use the
+/// opposite condition.
+/// 4. Implicit conversions of pointer to `bool` are replaced with explicit
+/// comparisons to `nullptr`.
+/// 5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
+/// 6. Object expressions with `explicit operator bool` conversion operators
+/// are replaced with explicit casts to `bool`.
+///
+/// Examples:
+/// 1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant
+/// parentheses and becomes `bool b = i < 0;`.
+///
+/// 2. The conditional return `if (!b) return false; return true;` has an
+/// implied double negation and becomes `return b;`.
+///
+/// 3. The conditional return `if (i < 0) return false; return true;` becomes
+/// `return i >= 0;`.
+/// The conditional return `if (i != 0) return false; return true;` becomes
+/// `return i == 0;`.
+///
+/// 4. The conditional return `if (p) return true; return false;` has an
+/// implicit conversion of a pointer to `bool` and becomes
+/// `return p != nullptr;`.
+/// The ternary assignment `bool b = (i & 1) ? true : false;` has an implicit
+/// conversion of `i & 1` to `bool` and becomes
+/// `bool b = static_cast<bool>(i & 1);`.
+///
+/// 5. The conditional return `if (i & 1) return true; else return false;` has
+/// an implicit conversion of an integer quantity `i & 1` to `bool` and becomes
+/// `return static_cast<bool>(i & 1);`
+///
+/// 6. Given `struct X { explicit operator bool(); };`, and an instance `x` of
+/// `struct X`, the conditional return `if (x) return true; return false;`
+/// becomes `return static_cast<bool>(x);`
///
/// When a conditional boolean return or assignment appears at the end of a
/// chain of `if`, `else if` statements, the conditional statement is left
@@ -77,6 +114,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 +143,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,275 @@
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: {{^}$}}
+
+bool if_implicit_bool_expr(int i) {
+ if (i & 1) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return static_cast<bool>(i & 1);{{$}}
+
+bool negated_if_implicit_bool_expr(int i) {
+ if (i - 1) {
+ return false;
+ } else {
+ return true;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return !static_cast<bool>(i - 1);{{$}}
+
+bool implicit_int(int i) {
+ if (i) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return static_cast<bool>(i);{{$}}
+
+bool explicit_bool(bool b) {
+ if (b) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return b;{{$}}
+
+class Implicit {
+public:
+ operator bool() {
+ return true;
+ }
+};
+
+bool object_bool_implicit_conversion(Implicit O) {
+ if (O) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return O;{{$}}
+
+bool negated_explicit_bool(bool b) {
+ if (!b) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return !b;{{$}}
+
+bool bitwise_complement_conversion(int i) {
+ if (~i) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return static_cast<bool>(~i);{{$}}
+
+bool logical_or(bool a, bool b) {
+ if (a || b) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return a || b;{{$}}
+
+bool logical_and(bool a, bool b) {
+ if (a && b) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return a && b;{{$}}
+
+class Comparable
+{
+public:
+ bool operator==(Comparable const &rhs) { return true; }
+ bool operator!=(Comparable const &rhs) { return false; }
+};
+
+bool comparable_objects() {
+ Comparable c;
+ Comparable d;
+ if (c == d) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return c == d;{{$}}
+
+bool negated_comparable_objects() {
+ Comparable c;
+ Comparable d;
+ if (c == d) {
+ return false;
+ } else {
+ return true;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}} return c != d;{{$}}
+
+struct X {
+ explicit operator bool();
+};
+
+void explicit_conversion_assignment(X x) {
+ bool y;
+ if (x) {
+ y = true;
+ } else {
+ y = false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{^ bool y;$}}
+// CHECK-FIXES: {{^}} y = static_cast<bool>(x);{{$}}
+
+void ternary_integer_condition(int i) {
+ bool b = i ? true : false;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: bool b = static_cast<bool>(i);{{$}}
+
+bool non_null_pointer_condition(int *p1) {
+ if (p1) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p1 != nullptr;{{$}}
+
+bool null_pointer_condition(int *p2) {
+ if (!p2) {
+ return true;
+ } else {
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p2 == nullptr;{{$}}
+
+bool negated_non_null_pointer_condition(int *p3) {
+ if (p3) {
+ return false;
+ } else {
+ return true;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p3 == nullptr;{{$}}
+
+bool negated_null_pointer_condition(int *p4) {
+ if (!p4) {
+ return false;
+ } else {
+ return true;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p4 != nullptr;{{$}}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
