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: clang-tidy/readability/SimplifyBooleanExprCheck.cpp =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -48,6 +48,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 +62,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 +74,11 @@ : Literal; } -internal::Matcher ReturnsBool(bool Value, StringRef Id = "") { - auto SimpleReturnsBool = returnStmt( - has(boolLiteral(equals(Value)).bind(Id.empty() ? "ignored" : Id))); +internal::Matcher returnsBool(bool Value, StringRef Id = "") { + auto SimpleReturnsBool = + returnStmt( + has(boolLiteral(equals(Value)).bind(Id.empty() ? "ignored" : Id))) + .bind("returns-bool"); return anyOf(SimpleReturnsBool, compoundStmt(statementCountIs(1), has(SimpleReturnsBool))); } @@ -118,6 +127,23 @@ : Text).str(); } +bool isFollowedBy(const CompoundStmt *Compound, const Stmt *Before, + const Stmt *After) { + if (Compound->size() < 2) { + return false; + } + + CompoundStmt::const_body_iterator BeforeIt = Compound->body_begin(); + CompoundStmt::const_body_iterator AfterIt = Compound->body_begin(); + for (++AfterIt; AfterIt != Compound->body_end(); ++BeforeIt, ++AfterIt) { + if (*BeforeIt == Before && *AfterIt == After) { + return true; + } + } + + return false; +} + } // namespace SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name, @@ -206,14 +232,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); } } @@ -251,6 +277,20 @@ ChainedConditionalAssignment); } +void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder, + bool Value, + StringRef Id) { + Finder->addMatcher( + compoundStmt( + allOf(hasAnySubstatement( + ifStmt( + hasThen(returnsBool(Value, CompoundId)), + unless(hasElse(stmt()))).bind(CompoundIfReturnsBoolId)), + hasAnySubstatement(returnStmt(has(boolLiteral(equals(!Value)))) + .bind(CompoundReturnId)))).bind(Id), + this); +} + void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { matchBoolBinOpExpr(Finder, true, "&&", RightExpressionId); matchBoolBinOpExpr(Finder, false, "||", RightExpressionId); @@ -283,6 +323,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 +365,12 @@ } else if (const auto *IfAssignNot = Result.Nodes.getNodeAs(IfAssignNotBoolId)) { replaceWithAssignment(Result, IfAssignNot, true); + } else if (const auto *Compound = + Result.Nodes.getNodeAs(CompoundBoolId)) { + replaceCompoundReturnWithCondition(Result, Compound); + } else if (const auto *Compound = + Result.Nodes.getNodeAs(CompoundNotBoolId)) { + replaceCompoundReturnWithCondition(Result, Compound, true); } } @@ -375,10 +424,28 @@ std::string Replacement = ("return " + Condition + Terminator).str(); SourceLocation Start = Result.Nodes.getNodeAs(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 *IfRet = Result.Nodes.getNodeAs(CompoundIfReturnsBoolId); + const auto *Ret = Result.Nodes.getNodeAs(CompoundReturnId); + + if (isFollowedBy(Compound, IfRet, Ret)) { + const Expr *Condition = IfRet->getCond(); + const auto *Lit = Result.Nodes.getNodeAs(CompoundId); + assert(Lit); + std::string Replacement = + "return " + replacementExpression(Result, Negated, Condition); + diag(Lit->getLocStart(), SimplifyConditionalReturnDiagnostic) + << FixItHint::CreateReplacement( + SourceRange(IfRet->getLocStart(), Ret->getLocEnd()), Replacement); + } +} + void SimplifyBooleanExprCheck::replaceWithAssignment( const MatchFinder::MatchResult &Result, const IfStmt *IfAssign, bool Negated) { 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 @@ -593,3 +593,45 @@ else b = 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: {{^}}}{{$}}