njames93 created this revision.
njames93 added reviewers: aaron.ballman, JonasToth, LegalizeAdulthood, alexfh.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Extend the check to catch these conditions:
if (e) return true; else return x; // return e || x;
if (e) return false; else return x; // return !e && x;
if (e) return true; return x; // return e || x;
if (e) return false; return x; // return !e && x;
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D133102
Files:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp
clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr.cpp
+++ 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;
Index: clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/simplify-bool-expr-case.cpp
+++ 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;
Index: clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
+++ 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``
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
+ <clang-tidy/checks/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
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -10,6 +10,8 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H
#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Stmt.h"
namespace clang {
namespace tidy {
@@ -50,6 +52,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);
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -324,14 +324,20 @@
using ExprAndBool = NodeAndBool<Expr>;
using DeclAndBool = NodeAndBool<Decl>;
+ static const ReturnStmt *getAsNonVoidReturn(const Stmt *S) {
+ if (const auto *RS = dyn_cast<ReturnStmt>(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<ReturnStmt>(S);
- if (!RS || !RS->getRetValue())
- return {};
- if (Optional<bool> Ret =
- getAsBoolLiteral(RS->getRetValue()->IgnoreImplicit(), false)) {
- return {RS->getRetValue(), *Ret};
+ const ReturnStmt *RS = getAsNonVoidReturn(S);
+ if (RS) {
+ if (Optional<bool> 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<IfStmt>(parent())) {
- Check->replaceWithReturnCondition(Context, If, ThenReturnBool.Item,
- ElseReturnBool.Bool);
+ if (Check->ChainedConditionalReturn ||
+ !isa_and_present<IfStmt>(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<IfStmt>(*First);
- ExprAndBool TrailingReturnBool = parseReturnLiteralBool(*Second);
- if (!TrailingReturnBool)
+ auto *Ret = dyn_cast<ReturnStmt>(*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<IfStmt>(*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<ReturnStmt>(*Second), TrailingReturnBool.Bool,
- If, ThenReturnBool.Item);
+ Context, cast<ReturnStmt>(*Second), *ReturnBoolValue, If,
+ ThenReturnBool.Item);
}
+ } else {
+ Check->replaceWithReturnComplexCondition(
+ Context, If, !ThenReturnBool.Bool, Ret->getRetValue());
}
}
+
} else if (isa<LabelStmt, CaseStmt, DefaultStmt>(*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<ReturnStmt>(*Second), TrailingReturnBool.Bool,
- SubIf, ThenReturnBool.Item);
+ if (!ThenReturnBool)
+ continue;
+ if (ReturnBoolValue) {
+ if (ThenReturnBool.Bool != *ReturnBoolValue) {
+ Check->replaceCompoundReturnWithCondition(
+ Context, cast<ReturnStmt>(*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<FixItHint> &Fixes,
+ const ASTContext &Ctx, const Expr *E,
+ Optional<BinaryOperatorKind> OuterBO);
+
+void SimplifyBooleanExprCheck::replaceWithReturnComplexCondition(
+ const ASTContext &Context, const IfStmt *If, bool Negated,
+ const Expr *ReturnValue) {
+ auto D = diag(If->getBeginLoc(), SimplifyConditionalReturnDiagnostic);
+ SmallVector<FixItHint, 8> 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<CompoundStmt>(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<FixItHint> &Fixes,
- const ASTContext &Ctx, const Expr *E,
- Optional<BinaryOperatorKind> 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.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
