diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -50,6 +50,10 @@ void replaceWithReturnCondition(const ASTContext &Context, const IfStmt *If, const Expr *BoolLiteral, bool Negated); + void replaceWithReturnComplexCondition(const ASTContext &Context, + const IfStmt *If, bool Negated, + const Expr *ReturnValue); + void replaceWithAssignment(const ASTContext &Context, const IfStmt *If, const Expr *Var, SourceLocation Loc, bool Negated); diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -324,14 +324,20 @@ using ExprAndBool = NodeAndBool; using DeclAndBool = NodeAndBool; + static const ReturnStmt *getAsNonVoidReturn(const Stmt *S) { + if (const auto *RS = dyn_cast(S); RS && RS->getRetValue()) + return RS; + return nullptr; + } + /// Detect's return (true|false|!true|!false); static ExprAndBool parseReturnLiteralBool(const Stmt *S) { - const auto *RS = dyn_cast(S); - if (!RS || !RS->getRetValue()) - return {}; - if (Optional Ret = - getAsBoolLiteral(RS->getRetValue()->IgnoreImplicit(), false)) { - return {RS->getRetValue(), *Ret}; + const ReturnStmt *RS = getAsNonVoidReturn(S); + if (RS) { + if (Optional Ret = + getAsBoolLiteral(RS->getRetValue()->IgnoreImplicit(), false)) { + return {RS->getRetValue(), *Ret}; + } } return {}; } @@ -374,16 +380,25 @@ /* * if (Cond) return true; else return false; -> return Cond; * if (Cond) return false; else return true; -> return !Cond; + * if (Cond) return true; else return X; -> return Cond || X; + * if (Cond) return false; else return X; -> return !Cond && X; */ if (ExprAndBool ThenReturnBool = checkSingleStatement(If->getThen(), parseReturnLiteralBool)) { - ExprAndBool ElseReturnBool = - checkSingleStatement(If->getElse(), parseReturnLiteralBool); - if (ElseReturnBool && ThenReturnBool.Bool != ElseReturnBool.Bool) { - if (Check->ChainedConditionalReturn || - !isa_and_nonnull(parent())) { - Check->replaceWithReturnCondition(Context, If, ThenReturnBool.Item, - ElseReturnBool.Bool); + if (Check->ChainedConditionalReturn || + !isa_and_present(parent())) { + if (const ReturnStmt *ElseReturn = + checkSingleStatement(If->getElse(), getAsNonVoidReturn)) { + if (auto RetBool = + getAsBoolLiteral(ElseReturn->getRetValue(), false)) { + if (*RetBool != ThenReturnBool.Bool) { + Check->replaceWithReturnCondition( + Context, If, ThenReturnBool.Item, *RetBool); + } + } else { + Check->replaceWithReturnComplexCondition( + Context, If, !ThenReturnBool.Bool, ElseReturn->getRetValue()); + } } } } else { @@ -457,29 +472,38 @@ Second != End; ++Second, ++First) { PrevIf = CurIf; CurIf = isa(*First); - ExprAndBool TrailingReturnBool = parseReturnLiteralBool(*Second); - if (!TrailingReturnBool) + auto *Ret = dyn_cast(*Second); + if (!Ret || !Ret->getRetValue()) continue; + auto ReturnBoolValue = + getAsBoolLiteral(Ret->getRetValue()->IgnoreImplicit(), false); if (CurIf) { /* * if (Cond) return true; return false; -> return Cond; * if (Cond) return false; return true; -> return !Cond; + * if (Cond) return true; return X; -> return Cond || X; + * if (Cond) return false; return X; -> return !Cond && X; */ auto *If = cast(*First); if (!If->hasInitStorage() && !If->hasVarStorage()) { ExprAndBool ThenReturnBool = checkSingleStatement(If->getThen(), parseReturnLiteralBool); - if (ThenReturnBool && - ThenReturnBool.Bool != TrailingReturnBool.Bool) { - if (Check->ChainedConditionalReturn || - (!PrevIf && If->getElse() == nullptr)) { + if (!ThenReturnBool || (!Check->ChainedConditionalReturn && + (PrevIf || If->getElse() != nullptr))) + continue; + if (ReturnBoolValue) { + if (ThenReturnBool.Bool != *ReturnBoolValue) { Check->replaceCompoundReturnWithCondition( - Context, cast(*Second), TrailingReturnBool.Bool, - If, ThenReturnBool.Item); + Context, cast(*Second), *ReturnBoolValue, If, + ThenReturnBool.Item); } + } else { + Check->replaceWithReturnComplexCondition( + Context, If, !ThenReturnBool.Bool, Ret->getRetValue()); } } + } else if (isa(*First)) { /* * (case X|label_X|default): if (Cond) return BoolLiteral; @@ -494,11 +518,17 @@ !SubIf->hasVarStorage()) { ExprAndBool ThenReturnBool = checkSingleStatement(SubIf->getThen(), parseReturnLiteralBool); - if (ThenReturnBool && - ThenReturnBool.Bool != TrailingReturnBool.Bool) { - Check->replaceCompoundReturnWithCondition( - Context, cast(*Second), TrailingReturnBool.Bool, - SubIf, ThenReturnBool.Item); + if (!ThenReturnBool) + continue; + if (ReturnBoolValue) { + if (ThenReturnBool.Bool != *ReturnBoolValue) { + Check->replaceCompoundReturnWithCondition( + Context, cast(*Second), *ReturnBoolValue, SubIf, + ThenReturnBool.Item); + } + } else { + Check->replaceWithReturnComplexCondition( + Context, SubIf, !ThenReturnBool.Bool, Ret->getRetValue()); } } } @@ -738,6 +768,31 @@ If->getSourceRange(), Replacement); } +static bool flipDemorganSide(SmallVectorImpl &Fixes, + const ASTContext &Ctx, const Expr *E, + Optional OuterBO); + +void SimplifyBooleanExprCheck::replaceWithReturnComplexCondition( + const ASTContext &Context, const IfStmt *If, bool Negated, + const Expr *ReturnValue) { + auto D = diag(If->getBeginLoc(), SimplifyConditionalReturnDiagnostic); + SmallVector Fixes; + Fixes.push_back(FixItHint::CreateReplacement( + {If->getBeginLoc(), If->getLParenLoc()}, "return ")); + if (Negated && flipDemorganSide(Fixes, Context, If->getCond(), + Negated ? BO_LAnd : BO_LOr)) + return; + Fixes.push_back(FixItHint::CreateReplacement( + {If->getRParenLoc(), ReturnValue->getBeginLoc().getLocWithOffset(-1)}, + Negated ? " && " : " || ")); + if (If->getElse()) { + if (const auto *CS = dyn_cast(If->getElse())) { + Fixes.push_back(FixItHint::CreateRemoval(CS->getRBracLoc())); + } + } + D << Fixes; +} + void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition( const ASTContext &Context, const ReturnStmt *Ret, bool Negated, const IfStmt *If, const Expr *ThenReturn) { @@ -780,10 +835,6 @@ return BO == BO_LAnd ? BO_LOr : BO_LAnd; } -static bool flipDemorganSide(SmallVectorImpl &Fixes, - const ASTContext &Ctx, const Expr *E, - Optional OuterBO); - /// Inverts \p BinOp, Removing \p Parens if they exist and are safe to remove. /// returns \c true if there is any issue building the Fixes, \c false /// otherwise. diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -147,6 +147,12 @@ The check now skips unions since in this case a default constructor with empty body is not equivalent to the explicitly defaulted one. +- Improved :doc:`readability-simplify-boolean-expr + ` check. + + The check can now transform ``if (Cond) return true; else return Something;`` + into ``return Cond || Something;`` and other similar cases. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst --- a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst @@ -24,10 +24,14 @@ ``if (false) t(); else f();`` ``f();`` ``if (e) return true; else return false;`` ``return e;`` ``if (e) return false; else return true;`` ``return !e;`` +``if (e) return true; else return x;`` ``return e || x;`` +``if (e) return false; else return x;`` ``return !e && x;`` ``if (e) b = true; else b = false;`` ``b = e;`` ``if (e) b = false; else b = true;`` ``b = !e;`` ``if (e) return true; return false;`` ``return e;`` ``if (e) return false; return true;`` ``return !e;`` +``if (e) return true; return x;`` ``return e || x;`` +``if (e) return false; return x;`` ``return !e && x;`` ``!(!a || b)`` ``a && !b`` ``!(a || !b)`` ``!a && b`` ``!(!a || !b)`` ``a && b`` diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp @@ -143,6 +143,20 @@ return false; return true; + case 18: + if (j > 10) + return true; + return j < 5; + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: {{.*}} in conditional return + // CHECK-FIXES: return j > 10 || j < 5; + + case 19: + if (j > 10) + return false; + return j > 5; + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: {{.*}} in conditional return + // CHECK-FIXES: return j <= 10 && j > 5; + case 100: { if (b == true) j = 10; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp @@ -357,10 +357,6 @@ if (i == j) return (i == 0); else return false; } -bool conditional_return_statements_else_expr(int i, int j) { - if (i == j) return true; else return (i == 0); -} - bool negated_conditional_return_statements(int i) { if (i == 0) return false; else return true; } @@ -387,6 +383,46 @@ // CHECK-FIXES-NEXT: {{^}} return i == 1;{{$}} // CHECK-FIXES-NEXT: {{^}$}} +bool getFallback(); + +bool conditional_return_complex(bool B) { + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement + // CHECK-FIXES: return B || getFallback(); + if (B) + return true; + else + return getFallback(); +} + +bool conditional_return_complex_compound(bool B) { + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement + // CHECK-FIXES: return B || getFallback(); + if (B) { + return true; + } else { + return getFallback(); + } +} + +bool conditional_return_complex_compound_negated(bool B) { + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement + // CHECK-FIXES: return !B && getFallback(); + if (B) { + return false; + } else { + return getFallback(); + } +} + +bool conditional_return_complex_negated(bool B) { + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: {{.*}} in conditional return statement + // CHECK-FIXES: return !B && getFallback(); + if (B) + return false; + else + return getFallback(); +} + bool negated_conditional_compound_return_statements(int i) { if (i == 1) { return false; @@ -745,6 +781,26 @@ // CHECK-FIXES: {{^ return i <= 10;$}} // CHECK-FIXES: {{^}$}} +bool simple_if_return_return_expr(int i) { + if (i > 10) + return true; + return i < 5; +} +// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: {{.*}} in conditional return +// CHECK-FIXES: {{^}}bool simple_if_return_return_expr(int i) {{{$}} +// CHECK-FIXES: {{^}} return i > 10 || i < 5;{{$}} +// CHECK-FIXES: {{^}$}} + +bool simple_if_return_return_expr_negated(int i) { + if (i > 10) + return false; + return i > 5; +} +// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: {{.*}} in conditional return +// CHECK-FIXES: {{^}}bool simple_if_return_return_expr_negated(int i) {{{$}} +// CHECK-FIXES: {{^}} return i <= 10 && i > 5;{{$}} +// CHECK-FIXES: {{^}$}} + bool if_implicit_bool_expr(int i) { if (i & 1) { return true;