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,9 @@ : 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 = "ignored") { + auto SimpleReturnsBool = + returnStmt(has(boolLiteral(equals(Value)).bind(Id))).bind("returns-bool"); return anyOf(SimpleReturnsBool, compoundStmt(statementCountIs(1), has(SimpleReturnsBool))); } @@ -110,6 +117,10 @@ return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator + " " + getText(Result, *BinOp->getRHS())).str(); } + } else if (const auto *UnOp = dyn_cast(E)) { + if (UnOp->getOpcode() == UO_LNot) { + return replacementExpression(Result, false, UnOp->getSubExpr()); + } } } StringRef Text = getText(Result, *E); @@ -118,6 +129,34 @@ : Text).str(); } +bool stmtReturnsBool(ReturnStmt *Ret, bool Negated, + const CXXBoolLiteralExpr *&Lit) { + if (auto *Bool = dyn_cast(Ret->getRetValue())) { + Lit = Bool; + return Bool->getValue() == !Negated; + } + return false; +} + +bool stmtReturnsBool(IfStmt *IfRet, bool Negated, + const CXXBoolLiteralExpr *&Lit) { + if (IfRet->getElse() != nullptr) { + return false; + } + + if (auto *Ret = dyn_cast(IfRet->getThen())) { + return stmtReturnsBool(Ret, Negated, Lit); + } else if (auto *Compound = dyn_cast(IfRet->getThen())) { + if (Compound->size() == 1) { + if (auto *CompoundRet = dyn_cast(Compound->body_back())) { + return stmtReturnsBool(CompoundRet, Negated, Lit); + } + } + } + + return false; +} + } // 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(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 +435,35 @@ 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 *Ret = Result.Nodes.getNodeAs(CompoundReturnId); + + CompoundStmt::const_body_iterator BeforeIt = Compound->body_begin(); + CompoundStmt::const_body_iterator AfterIt = Compound->body_begin(); + for (++AfterIt; AfterIt != Compound->body_end() && *BeforeIt != Ret; + ++BeforeIt, ++AfterIt) { + if (auto *If = dyn_cast(*BeforeIt)) { + const CXXBoolLiteralExpr *Lit = nullptr; + if (*AfterIt == Ret && stmtReturnsBool(If, Negated, Lit)) { + const Expr *Condition = If->getCond(); + assert(Lit); + std::string Replacement = + "return " + replacementExpression(Result, Negated, Condition); + diag(Lit->getLocStart(), SimplifyConditionalReturnDiagnostic) + << FixItHint::CreateReplacement( + SourceRange(If->getLocStart(), Ret->getLocEnd()), Replacement); + return; + } + } + } +} + 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 @@ -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,107 @@ 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: {{^}}}{{$}} + +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: {{^}}}{{$}}