LegalizeAdulthood updated this revision to Diff 403390.
LegalizeAdulthood added a comment.
Update from review comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56303/new/
https://reviews.llvm.org/D56303
Files:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
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
clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
@@ -1,6 +1,9 @@
+#include "../../clang/unittests/ASTMatchers/ASTMatchersTest.h"
#include "ClangTidyTest.h"
#include "readability/BracesAroundStatementsCheck.h"
#include "readability/NamespaceCommentCheck.h"
+#include "readability/SimplifyBooleanExprMatchers.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "gtest/gtest.h"
namespace clang {
@@ -9,6 +12,123 @@
using readability::BracesAroundStatementsCheck;
using readability::NamespaceCommentCheck;
+using namespace ast_matchers;
+
+TEST_P(ASTMatchersTest, HasCaseSubstatement) {
+ EXPECT_TRUE(matches(
+ "void f() { switch (1) { case 1: return; break; default: break; } }",
+ traverse(TK_AsIs, caseStmt(hasSubstatement(returnStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasDefaultSubstatement) {
+ EXPECT_TRUE(matches(
+ "void f() { switch (1) { case 1: return; break; default: break; } }",
+ traverse(TK_AsIs, defaultStmt(hasSubstatement(breakStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasLabelSubstatement) {
+ EXPECT_TRUE(
+ matches("void f() { while (1) { bar: break; foo: return; } }",
+ traverse(TK_AsIs, labelStmt(hasSubstatement(breakStmt())))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceSimple) {
+ const char *Text = "int f() { int x = 5; if (x < 0) return 1; return 0; }";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), labelStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(returnStmt(), ifStmt()))));
+ EXPECT_FALSE(matches(
+ Text, compoundStmt(hasSubstatementSequence(switchStmt(), labelStmt()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceAlmost) {
+ const char *Text = R"code(
+int f() {
+ int x = 5;
+ if (x < 10)
+ ;
+ if (x < 0)
+ return 1;
+ return 0;
+}
+)code";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_TRUE(
+ matches(Text, compoundStmt(hasSubstatementSequence(ifStmt(), ifStmt()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceComplex) {
+ const char *Text = R"code(
+int f() {
+ int x = 5;
+ if (x < 10)
+ x -= 10;
+ if (x < 0)
+ return 1;
+ return 0;
+}
+)code";
+ EXPECT_TRUE(matches(
+ Text, compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))));
+ EXPECT_FALSE(
+ matches(Text, compoundStmt(hasSubstatementSequence(ifStmt(), expr()))));
+}
+
+TEST_P(ASTMatchersTest, HasSubstatementSequenceExpression) {
+ const char *Text = R"code(
+int f() {
+ return ({ int x = 5;
+ int result;
+ if (x < 10)
+ x -= 10;
+ if (x < 0)
+ result = 1;
+ else
+ result = 0;
+ result;
+ });
+ }
+)code";
+ EXPECT_TRUE(
+ matches(Text, stmtExpr(hasSubstatementSequence(ifStmt(), expr()))));
+ EXPECT_FALSE(
+ matches(Text, stmtExpr(hasSubstatementSequence(ifStmt(), returnStmt()))));
+}
+
+// Copied from ASTMatchersTests
+static std::vector<TestClangConfig> allTestClangConfigs() {
+ std::vector<TestClangConfig> all_configs;
+ for (TestLanguage lang : {Lang_C89, Lang_C99, Lang_CXX03, Lang_CXX11,
+ Lang_CXX14, Lang_CXX17, Lang_CXX20}) {
+ TestClangConfig config;
+ config.Language = lang;
+
+ // Use an unknown-unknown triple so we don't instantiate the full system
+ // toolchain. On Linux, instantiating the toolchain involves stat'ing
+ // large portions of /usr/lib, and this slows down not only this test, but
+ // all other tests, via contention in the kernel.
+ //
+ // FIXME: This is a hack to work around the fact that there's no way to do
+ // the equivalent of runToolOnCodeWithArgs without instantiating a full
+ // Driver. We should consider having a function, at least for tests, that
+ // invokes cc1.
+ config.Target = "i386-unknown-unknown";
+ all_configs.push_back(config);
+
+ // Windows target is interesting to test because it enables
+ // `-fdelayed-template-parsing`.
+ config.Target = "x86_64-pc-win32-msvc";
+ all_configs.push_back(config);
+ }
+ return all_configs;
+}
+
+INSTANTIATE_TEST_SUITE_P(ASTMatchersTests, ASTMatchersTest,
+ testing::ValuesIn(allTestClangConfigs()));
TEST(NamespaceCommentCheckTest, Basic) {
EXPECT_EQ("namespace i {\n} // namespace i",
@@ -488,9 +608,9 @@
Opts.CheckOptions["test-check-0.ShortStatementLines"] = "1";
StringRef Input = "const char *f() {\n"
- " if (true) return \"\";\n"
- " return \"abc\";\n"
- "}\n";
+ " if (true) return \"\";\n"
+ " return \"abc\";\n"
+ "}\n";
EXPECT_NO_CHANGES_WITH_OPTS(BracesAroundStatementsCheck, Opts, Input);
EXPECT_EQ("const char *f() {\n"
" if (true) { return \"\";\n"
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===================================================================
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -42,6 +42,7 @@
clangFrontend
clangLex
clangSerialization
+ clangTesting
clangTooling
clangToolingCore
clangTransformer
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
@@ -581,19 +581,19 @@
// Unchanged: multiple statements.
bool g;
if (j > 10)
- f = true;
+ g = true;
else {
j = 20;
- f = false;
+ g = false;
}
// Unchanged: multiple statements.
bool h;
if (j > 10) {
j = 10;
- f = true;
+ h = true;
} else
- f = false;
+ h = false;
}
// Unchanged: chained return statements, but ChainedConditionalReturn not set.
Index: clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,744 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+
+ case 1:
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 2:
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+
+ case 3:
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 4:
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+
+ case 5:
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+
+ case 6:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+
+ case 7:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+
+ case 8:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+
+ case 9:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+
+ case 10:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+
+ case 11:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+
+ case 12:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+
+ case 13:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+
+ case 14:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+
+ case 15:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+
+ case 16:
+ if (j > 10)
+ return true;
+ return true;
+ return false;
+
+ case 17:
+ if (j > 10)
+ return false;
+ return false;
+ return true;
+
+ case 100: {
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+
+ case 101: {
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 102: {
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+
+ case 103: {
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 104: {
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+
+ case 105: {
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+
+ case 106: {
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+ }
+
+ case 107: {
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+ }
+
+ case 108: {
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+ }
+
+ case 109: {
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+
+ case 110: {
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+
+ case 111: {
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+
+ case 112: {
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+
+ case 113: {
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+
+ case 114: {
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+
+ case 115: {
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+
+ case 116: {
+ return false;
+ if (j > 10)
+ return true;
+ }
+
+ case 117: {
+ return true;
+ if (j > 10)
+ return false;
+ }
+ }
+
+ return j > 0;
+}
+
+bool default_stmt0(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b == true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+ return false;
+}
+
+bool default_stmt1(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b == false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt2(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b && true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ }
+ return false;
+}
+
+bool default_stmt3(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b && false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt4(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b || true)
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+ return false;
+}
+
+bool default_stmt5(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (b || false)
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ }
+ return false;
+}
+
+bool default_stmt6(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+ }
+ return false;
+}
+
+bool default_stmt7(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+ }
+ return false;
+}
+
+bool default_stmt8(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ // CHECK-FIXES-NEXT: {{break;$}}
+ }
+ return false;
+}
+
+bool default_stmt9(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ break;
+ // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ // CHECK-FIXES-NEXT: {{break;}}
+ }
+ return false;
+}
+
+bool default_stmt10(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+ }
+ return false;
+}
+
+bool default_stmt11(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+ }
+ return false;
+}
+
+bool default_stmt12(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+ return false;
+}
+
+bool default_stmt13(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+ }
+ return false;
+}
+
+bool default_stmt14(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+ }
+ return false;
+}
+
+bool default_stmt15(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+ }
+ return false;
+}
+
+bool default_stmt16(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return false;
+
+ default:
+ if (j > 10)
+ return true;
+ }
+ return false;
+}
+
+bool default_stmt17(int i, int j, bool b) {
+ switch (i) {
+ case 0:
+ return true;
+
+ default:
+ if (j > 10)
+ return false;
+ }
+ return false;
+}
+
+bool label_stmt0(int i, int j, bool b) {
+label:
+ if (b == true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt1(int i, int j, bool b) {
+label:
+ if (b == false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(!b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt2(int i, int j, bool b) {
+label:
+ if (b && true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt3(int i, int j, bool b) {
+label:
+ if (b && false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(false\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt4(int i, int j, bool b) {
+label:
+ if (b || true)
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(true\)}}
+ // CHECK-FIXES-NEXT: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt5(int i, int j, bool b) {
+label:
+ if (b || false)
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} to boolean operator
+ // CHECK-FIXES: {{if \(b\)}}
+ // CHECK-FIXES-NEXT: {{j = -20;}}
+ return false;
+}
+
+bool label_stmt6(int i, int j, bool b) {
+label:
+ return i > 0 ? true : false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i > 0;}}
+}
+
+bool label_stmt7(int i, int j, bool b) {
+label:
+ return i > 0 ? false : true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: {{.*}} in ternary expression result
+ // CHECK-FIXES: {{return i <= 0;}}
+}
+
+bool label_stmt8(int i, int j, bool b) {
+label:
+ if (true)
+ j = 10;
+ else
+ j = -20;
+ // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;$}}
+ return false;
+}
+
+bool label_stmt9(int i, int j, bool b) {
+label:
+ if (false)
+ j = -20;
+ else
+ j = 10;
+ // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: {{.*}} in if statement condition
+ // CHECK-FIXES: {{j = 10;}}
+ return false;
+}
+
+bool label_stmt10(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return true;
+ else
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j > 10;}}
+}
+
+bool label_stmt11(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return false;
+ else
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-3]]:12: warning: {{.*}} in conditional return statement
+ // CHECK-FIXES: {{return j <= 10;}}
+}
+
+bool label_stmt12(int i, int j, bool b) {
+label:
+ if (j > 10)
+ b = true;
+ else
+ b = false;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j > 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+}
+
+bool label_stmt13(int i, int j, bool b) {
+label:
+ if (j > 10)
+ b = false;
+ else
+ b = true;
+ return b;
+ // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: {{.*}} in conditional assignment
+ // CHECK-FIXES: {{b = j <= 10;}}
+ // CHECK-FIXES-NEXT: {{return b;}}
+}
+
+bool label_stmt14(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return true;
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} in conditional return
+ // FIXES: {{return j > 10;}}
+}
+
+bool label_stmt15(int i, int j, bool b) {
+label:
+ if (j > 10)
+ return false;
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: {{.*}} in conditional return
+ // FIXES: {{return j <= 10;}}
+}
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprMatchers.h
@@ -0,0 +1,67 @@
+//===-- SimplifyBooleanExprMatchers.h - clang-tidy ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+
+namespace clang {
+namespace ast_matchers {
+
+/// Matches the substatement associated with a case, default or label statement.
+///
+/// Given
+/// \code
+/// switch (1) { case 1: break; case 2: return; break; default: return; break;
+/// }
+/// foo: return;
+/// bar: break;
+/// \endcode
+///
+/// caseStmt(hasSubstatement(returnStmt()))
+/// matches "case 2: return;"
+/// defaultStmt(hasSubstatement(returnStmt()))
+/// matches "default: return;"
+/// labelStmt(hasSubstatement(breakStmt()))
+/// matches "bar: break;"
+AST_POLYMORPHIC_MATCHER_P(hasSubstatement,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(CaseStmt, DefaultStmt,
+ LabelStmt),
+ internal::Matcher<Stmt>, InnerMatcher) {
+ return InnerMatcher.matches(*Node.getSubStmt(), Finder, Builder);
+}
+
+/// Matches two consecutive statements within a compound statement.
+///
+/// Given
+/// \code
+/// { if (x > 0) return true; return false; }
+/// \endcode
+/// compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))
+/// matches '{ if (x > 0) return true; return false; }'
+AST_POLYMORPHIC_MATCHER_P2(hasSubstatementSequence,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(CompoundStmt,
+ StmtExpr),
+ internal::Matcher<Stmt>, InnerMatcher1,
+ internal::Matcher<Stmt>, InnerMatcher2) {
+ if (const CompoundStmt *CS = CompoundStmtMatcher<NodeType>::get(Node)) {
+ auto It = matchesFirstInPointerRange(InnerMatcher1, CS->body_begin(),
+ CS->body_end(), Finder, Builder);
+ while (It != CS->body_end()) {
+ ++It;
+ if (It == CS->body_end())
+ return false;
+ if (InnerMatcher2.matches(**It, Finder, Builder))
+ return true;
+ It = matchesFirstInPointerRange(InnerMatcher1, It, CS->body_end(), Finder,
+ Builder);
+ }
+ }
+ return false;
+}
+
+} // namespace ast_matchers
+} // namespace clang
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
@@ -41,7 +41,7 @@
StringRef BooleanId);
void matchTernaryResult(ast_matchers::MatchFinder *Finder, bool Value,
- StringRef TernaryId);
+ StringRef Id);
void matchIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
StringRef Id);
@@ -52,30 +52,51 @@
void matchCompoundIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
StringRef Id);
+ void matchCaseIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
+ void matchDefaultIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
+ void matchLabelIfReturnsBool(ast_matchers::MatchFinder *Finder, bool Value,
+ StringRef Id);
+
void
replaceWithThenStatement(const ast_matchers::MatchFinder::MatchResult &Result,
const Expr *BoolLiteral);
void
replaceWithElseStatement(const ast_matchers::MatchFinder::MatchResult &Result,
- const Expr *FalseConditionRemoved);
+ const Expr *BoolLiteral);
void
replaceWithCondition(const ast_matchers::MatchFinder::MatchResult &Result,
- const ConditionalOperator *Ternary,
- bool Negated = false);
+ const ConditionalOperator *Ternary, bool Negated);
void replaceWithReturnCondition(
const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If,
- bool Negated = false);
+ bool Negated);
void
replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
- const IfStmt *If, bool Negated = false);
+ const IfStmt *If, bool Negated);
void replaceCompoundReturnWithCondition(
const ast_matchers::MatchFinder::MatchResult &Result,
- const CompoundStmt *Compound, bool Negated = false);
+ const CompoundStmt *Compound, bool Negated);
+
+ void replaceCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated,
+ const IfStmt *If);
+
+ void replaceCaseCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
+
+ void replaceDefaultCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
+
+ void replaceLabelCompoundReturnWithCondition(
+ const ast_matchers::MatchFinder::MatchResult &Result, bool Negated);
void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result,
SourceLocation Loc, StringRef Description,
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
@@ -7,10 +7,10 @@
//===----------------------------------------------------------------------===//
#include "SimplifyBooleanExprCheck.h"
+#include "SimplifyBooleanExprMatchers.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Lex/Lexer.h"
-#include <cassert>
#include <string>
#include <utility>
@@ -33,33 +33,44 @@
return getText(Result, Node.getSourceRange());
}
-const char ConditionThenStmtId[] = "if-bool-yields-then";
-const char ConditionElseStmtId[] = "if-bool-yields-else";
-const char TernaryId[] = "ternary-bool-yields-condition";
-const char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
-const char IfReturnsBoolId[] = "if-return";
-const char IfReturnsNotBoolId[] = "if-not-return";
-const char ThenLiteralId[] = "then-literal";
-const char IfAssignVariableId[] = "if-assign-lvalue";
-const char IfAssignLocId[] = "if-assign-loc";
-const char IfAssignBoolId[] = "if-assign";
-const char IfAssignNotBoolId[] = "if-assign-not";
-const char IfAssignVarId[] = "if-assign-var";
-const char CompoundReturnId[] = "compound-return";
-const char CompoundBoolId[] = "compound-bool";
-const char CompoundNotBoolId[] = "compound-bool-not";
-
-const char IfStmtId[] = "if";
-
-const char SimplifyOperatorDiagnostic[] =
+} // namespace
+
+static constexpr char ConditionThenStmtId[] = "if-bool-yields-then";
+static constexpr char ConditionElseStmtId[] = "if-bool-yields-else";
+static constexpr char TernaryId[] = "ternary-bool-yields-condition";
+static constexpr char TernaryNegatedId[] = "ternary-bool-yields-not-condition";
+static constexpr char IfReturnsBoolId[] = "if-return";
+static constexpr char IfReturnsNotBoolId[] = "if-not-return";
+static constexpr char ThenLiteralId[] = "then-literal";
+static constexpr char IfAssignVariableId[] = "if-assign-lvalue";
+static constexpr char IfAssignLocId[] = "if-assign-loc";
+static constexpr char IfAssignBoolId[] = "if-assign";
+static constexpr char IfAssignNotBoolId[] = "if-assign-not";
+static constexpr char IfAssignVarId[] = "if-assign-var";
+static constexpr char CompoundReturnId[] = "compound-return";
+static constexpr char CompoundIfId[] = "compound-if";
+static constexpr char CompoundBoolId[] = "compound-bool";
+static constexpr char CompoundNotBoolId[] = "compound-bool-not";
+static constexpr char CaseId[] = "case";
+static constexpr char CaseCompoundBoolId[] = "case-compound-bool";
+static constexpr char CaseCompoundNotBoolId[] = "case-compound-bool-not";
+static constexpr char DefaultId[] = "default";
+static constexpr char DefaultCompoundBoolId[] = "default-compound-bool";
+static constexpr char DefaultCompoundNotBoolId[] = "default-compound-bool-not";
+static constexpr char LabelId[] = "label";
+static constexpr char LabelCompoundBoolId[] = "label-compound-bool";
+static constexpr char LabelCompoundNotBoolId[] = "label-compound-bool-not";
+static constexpr char IfStmtId[] = "if";
+
+static constexpr char SimplifyOperatorDiagnostic[] =
"redundant boolean literal supplied to boolean operator";
-const char SimplifyConditionDiagnostic[] =
+static constexpr char SimplifyConditionDiagnostic[] =
"redundant boolean literal in if statement condition";
-const char SimplifyConditionalReturnDiagnostic[] =
+static constexpr char SimplifyConditionalReturnDiagnostic[] =
"redundant boolean literal in conditional return statement";
-const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
- StringRef Id) {
+static const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
+ StringRef Id) {
if (const Expr *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id))
return Literal->getBeginLoc().isMacroID() ? nullptr : Literal;
if (const auto *Negated = Result.Nodes.getNodeAs<UnaryOperator>(Id)) {
@@ -70,21 +81,22 @@
return nullptr;
}
-internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
+static internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
return expr(
anyOf(cxxBoolLiteral(equals(Value)),
unaryOperator(hasUnaryOperand(cxxBoolLiteral(equals(!Value))),
hasOperatorName("!"))));
}
-internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
+static internal::Matcher<Stmt> returnsBool(bool Value,
+ StringRef Id = "ignored") {
auto SimpleReturnsBool = returnStmt(has(literalOrNegatedBool(Value).bind(Id)))
.bind("returns-bool");
return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
}
-bool needsParensAfterUnaryNegation(const Expr *E) {
+static bool needsParensAfterUnaryNegation(const Expr *E) {
E = E->IgnoreImpCasts();
if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
return true;
@@ -96,10 +108,10 @@
return false;
}
-std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
+static std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
{BO_LT, BO_GE}, {BO_GT, BO_LE}, {BO_EQ, BO_NE}};
-StringRef negatedOperator(const BinaryOperator *BinOp) {
+static StringRef negatedOperator(const BinaryOperator *BinOp) {
const BinaryOperatorKind Opcode = BinOp->getOpcode();
for (auto NegatableOp : Opposites) {
if (Opcode == NegatableOp.first)
@@ -107,28 +119,28 @@
if (Opcode == NegatableOp.second)
return BinOp->getOpcodeStr(NegatableOp.first);
}
- return StringRef();
+ return {};
}
-std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
+static std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
{OO_EqualEqual, "=="}, {OO_ExclaimEqual, "!="}, {OO_Less, "<"},
{OO_GreaterEqual, ">="}, {OO_Greater, ">"}, {OO_LessEqual, "<="}};
-StringRef getOperatorName(OverloadedOperatorKind OpKind) {
+static StringRef getOperatorName(OverloadedOperatorKind OpKind) {
for (auto Name : OperatorNames) {
if (Name.first == OpKind)
return Name.second;
}
- return StringRef();
+ return {};
}
-std::pair<OverloadedOperatorKind, OverloadedOperatorKind> OppositeOverloads[] =
- {{OO_EqualEqual, OO_ExclaimEqual},
- {OO_Less, OO_GreaterEqual},
- {OO_Greater, OO_LessEqual}};
+static std::pair<OverloadedOperatorKind, OverloadedOperatorKind>
+ OppositeOverloads[] = {{OO_EqualEqual, OO_ExclaimEqual},
+ {OO_Less, OO_GreaterEqual},
+ {OO_Greater, OO_LessEqual}};
-StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
+static StringRef negatedOperator(const CXXOperatorCallExpr *OpCall) {
const OverloadedOperatorKind Opcode = OpCall->getOperator();
for (auto NegatableOp : OppositeOverloads) {
if (Opcode == NegatableOp.first)
@@ -136,17 +148,17 @@
if (Opcode == NegatableOp.second)
return getOperatorName(NegatableOp.first);
}
- return StringRef();
+ return {};
}
-std::string asBool(StringRef Text, bool NeedsStaticCast) {
+static std::string asBool(StringRef Text, bool NeedsStaticCast) {
if (NeedsStaticCast)
return ("static_cast<bool>(" + Text + ")").str();
return std::string(Text);
}
-bool needsNullPtrComparison(const Expr *E) {
+static bool needsNullPtrComparison(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
return ImpCast->getCastKind() == CK_PointerToBoolean ||
ImpCast->getCastKind() == CK_MemberPointerToBoolean;
@@ -154,14 +166,14 @@
return false;
}
-bool needsZeroComparison(const Expr *E) {
+static bool needsZeroComparison(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
return ImpCast->getCastKind() == CK_IntegralToBoolean;
return false;
}
-bool needsStaticCast(const Expr *E) {
+static bool needsStaticCast(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
ImpCast->getSubExpr()->getType()->isBooleanType()) {
@@ -180,9 +192,9 @@
return !E->getType()->isBooleanType();
}
-std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated,
- const char *Constant) {
+static std::string
+compareExpressionToConstant(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated, const char *Constant) {
E = E->IgnoreImpCasts();
const std::string ExprText =
(isa<BinaryOperator>(E) ? ("(" + getText(Result, *E) + ")")
@@ -191,20 +203,22 @@
return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant;
}
-std::string compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated) {
+static std::string
+compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated) {
const char *NullPtr =
Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
return compareExpressionToConstant(Result, E, Negated, NullPtr);
}
-std::string compareExpressionToZero(const MatchFinder::MatchResult &Result,
- const Expr *E, bool Negated) {
+static std::string
+compareExpressionToZero(const MatchFinder::MatchResult &Result, const Expr *E,
+ bool Negated) {
return compareExpressionToConstant(Result, E, Negated, "0");
}
-std::string replacementExpression(const MatchFinder::MatchResult &Result,
- bool Negated, const Expr *E) {
+static std::string replacementExpression(const MatchFinder::MatchResult &Result,
+ bool Negated, const Expr *E) {
E = E->IgnoreParenBaseCasts();
if (const auto *EC = dyn_cast<ExprWithCleanups>(E))
E = EC->getSubExpr();
@@ -281,7 +295,7 @@
return asBool(getText(Result, *E), NeedsStaticCast);
}
-const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
+static const Expr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
if (Bool->getValue() == !Negated)
return Bool;
@@ -299,7 +313,7 @@
return nullptr;
}
-const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
+static const Expr *stmtReturnsBool(const IfStmt *IfRet, bool Negated) {
if (IfRet->getElse() != nullptr)
return nullptr;
@@ -316,8 +330,8 @@
return nullptr;
}
-bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
- CharSourceRange CharRange) {
+static bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
+ CharSourceRange CharRange) {
std::string ReplacementText =
Lexer::getSourceText(CharRange, *Result.SourceManager,
Result.Context->getLangOpts())
@@ -336,20 +350,18 @@
return false;
}
-} // namespace
-
class SimplifyBooleanExprCheck::Visitor : public RecursiveASTVisitor<Visitor> {
- public:
+public:
Visitor(SimplifyBooleanExprCheck *Check,
const MatchFinder::MatchResult &Result)
: Check(Check), Result(Result) {}
- bool VisitBinaryOperator(BinaryOperator *Op) {
+ bool VisitBinaryOperator(const BinaryOperator *Op) const {
Check->reportBinOp(Result, Op);
return true;
}
- private:
+private:
SimplifyBooleanExprCheck *Check;
const MatchFinder::MatchResult &Result;
};
@@ -361,7 +373,7 @@
ChainedConditionalAssignment(
Options.get("ChainedConditionalAssignment", false)) {}
-bool containsBoolLiteral(const Expr *E) {
+static bool containsBoolLiteral(const Expr *E) {
if (!E)
return false;
E = E->IgnoreParenImpCasts();
@@ -381,10 +393,10 @@
const auto *RHS = Op->getRHS()->IgnoreParenImpCasts();
const CXXBoolLiteralExpr *Bool;
- const Expr *Other = nullptr;
- if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)))
+ const Expr *Other;
+ if ((Bool = dyn_cast<CXXBoolLiteralExpr>(LHS)) != nullptr)
Other = RHS;
- else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)))
+ else if ((Bool = dyn_cast<CXXBoolLiteralExpr>(RHS)) != nullptr)
Other = LHS;
else
return;
@@ -408,34 +420,32 @@
};
switch (Op->getOpcode()) {
- case BO_LAnd:
- if (BoolValue) {
- // expr && true -> expr
- ReplaceWithExpression(Other, /*Negated=*/false);
- } else {
- // expr && false -> false
- ReplaceWithExpression(Bool, /*Negated=*/false);
- }
- break;
- case BO_LOr:
- if (BoolValue) {
- // expr || true -> true
- ReplaceWithExpression(Bool, /*Negated=*/false);
- } else {
- // expr || false -> expr
- ReplaceWithExpression(Other, /*Negated=*/false);
- }
- break;
- case BO_EQ:
- // expr == true -> expr, expr == false -> !expr
- ReplaceWithExpression(Other, /*Negated=*/!BoolValue);
- break;
- case BO_NE:
- // expr != true -> !expr, expr != false -> expr
- ReplaceWithExpression(Other, /*Negated=*/BoolValue);
- break;
- default:
- break;
+ case BO_LAnd:
+ if (BoolValue)
+ // expr && true -> expr
+ ReplaceWithExpression(Other, /*Negated=*/false);
+ else
+ // expr && false -> false
+ ReplaceWithExpression(Bool, /*Negated=*/false);
+ break;
+ case BO_LOr:
+ if (BoolValue)
+ // expr || true -> true
+ ReplaceWithExpression(Bool, /*Negated=*/false);
+ else
+ // expr || false -> expr
+ ReplaceWithExpression(Other, /*Negated=*/false);
+ break;
+ case BO_EQ:
+ // expr == true -> expr, expr == false -> !expr
+ ReplaceWithExpression(Other, /*Negated=*/!BoolValue);
+ break;
+ case BO_NE:
+ // expr != true -> !expr, expr != false -> expr
+ ReplaceWithExpression(Other, /*Negated=*/BoolValue);
+ break;
+ default:
+ break;
}
}
@@ -449,12 +459,11 @@
}
void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
- bool Value,
- StringRef TernaryId) {
+ bool Value, StringRef Id) {
Finder->addMatcher(
conditionalOperator(hasTrueExpression(literalOrNegatedBool(Value)),
hasFalseExpression(literalOrNegatedBool(!Value)))
- .bind(TernaryId),
+ .bind(Id),
this);
}
@@ -499,17 +508,66 @@
this);
}
+static internal::Matcher<Stmt> ifReturnValue(bool Value) {
+ return ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))
+ .bind(CompoundIfId);
+}
+
+static internal::Matcher<Stmt> returnNotValue(bool Value) {
+ return returnStmt(has(literalOrNegatedBool(!Value))).bind(CompoundReturnId);
+}
+
void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
bool Value,
StringRef Id) {
- Finder->addMatcher(
- compoundStmt(
- hasAnySubstatement(
- ifStmt(hasThen(returnsBool(Value)), unless(hasElse(stmt())))),
- hasAnySubstatement(returnStmt(has(literalOrNegatedBool(!Value)))
- .bind(CompoundReturnId)))
- .bind(Id),
- this);
+ if (ChainedConditionalReturn)
+ Finder->addMatcher(
+ compoundStmt(hasSubstatementSequence(ifReturnValue(Value),
+ returnNotValue(Value)))
+ .bind(Id),
+ this);
+ else
+ Finder->addMatcher(
+ compoundStmt(hasSubstatementSequence(ifStmt(hasThen(returnsBool(Value)),
+ unless(hasElse(stmt())),
+ unless(hasParent(ifStmt())))
+ .bind(CompoundIfId),
+ returnNotValue(Value)))
+ .bind(Id),
+ this);
+}
+
+void SimplifyBooleanExprCheck::matchCaseIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> CaseStmt =
+ caseStmt(hasSubstatement(ifReturnValue(Value))).bind(CaseId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(CaseStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
+}
+
+void SimplifyBooleanExprCheck::matchDefaultIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> DefaultStmt =
+ defaultStmt(hasSubstatement(ifReturnValue(Value))).bind(DefaultId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(DefaultStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(switchStmt(has(CompoundStmt)), this);
+}
+
+void SimplifyBooleanExprCheck::matchLabelIfReturnsBool(MatchFinder *Finder,
+ bool Value,
+ StringRef Id) {
+ internal::Matcher<Stmt> LabelStmt =
+ labelStmt(hasSubstatement(ifReturnValue(Value))).bind(LabelId);
+ internal::Matcher<Stmt> CompoundStmt =
+ compoundStmt(hasSubstatementSequence(LabelStmt, returnNotValue(Value)))
+ .bind(Id);
+ Finder->addMatcher(CompoundStmt, this);
}
void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
@@ -535,6 +593,15 @@
matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
+
+ matchCaseIfReturnsBool(Finder, true, CaseCompoundBoolId);
+ matchCaseIfReturnsBool(Finder, false, CaseCompoundNotBoolId);
+
+ matchDefaultIfReturnsBool(Finder, true, DefaultCompoundBoolId);
+ matchDefaultIfReturnsBool(Finder, false, DefaultCompoundNotBoolId);
+
+ matchLabelIfReturnsBool(Finder, true, LabelCompoundBoolId);
+ matchLabelIfReturnsBool(Finder, false, LabelCompoundNotBoolId);
}
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
@@ -548,33 +615,48 @@
replaceWithElseStatement(Result, FalseConditionRemoved);
else if (const auto *Ternary =
Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
- replaceWithCondition(Result, Ternary);
+ replaceWithCondition(Result, Ternary, false);
else if (const auto *TernaryNegated =
Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
replaceWithCondition(Result, TernaryNegated, true);
else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
- replaceWithReturnCondition(Result, If);
+ replaceWithReturnCondition(Result, If, false);
else if (const auto *IfNot =
Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
replaceWithReturnCondition(Result, IfNot, true);
else if (const auto *IfAssign =
Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
- replaceWithAssignment(Result, IfAssign);
+ replaceWithAssignment(Result, IfAssign, false);
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 =
+ replaceCompoundReturnWithCondition(Result, Compound, false);
+ else if (const auto *CompoundNot =
Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
- replaceCompoundReturnWithCondition(Result, Compound, true);
-}
-
-void SimplifyBooleanExprCheck::issueDiag(
- const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc,
- StringRef Description, SourceRange ReplacementRange,
- StringRef Replacement) {
+ replaceCompoundReturnWithCondition(Result, CompoundNot, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(CaseCompoundNotBoolId))
+ replaceCaseCompoundReturnWithCondition(Result, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(DefaultCompoundNotBoolId))
+ replaceDefaultCompoundReturnWithCondition(Result, true);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundBoolId))
+ replaceLabelCompoundReturnWithCondition(Result, false);
+ else if (Result.Nodes.getNodeAs<CompoundStmt>(LabelCompoundNotBoolId))
+ replaceLabelCompoundReturnWithCondition(Result, true);
+ else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
+ Visitor(this, Result).TraverseDecl(const_cast<Decl *>(TU));
+}
+
+void SimplifyBooleanExprCheck::issueDiag(const MatchFinder::MatchResult &Result,
+ SourceLocation Loc,
+ StringRef Description,
+ SourceRange ReplacementRange,
+ StringRef Replacement) {
CharSourceRange CharRange =
Lexer::makeFileCharRange(CharSourceRange::getTokenRange(ReplacementRange),
*Result.SourceManager, getLangOpts());
@@ -585,19 +667,19 @@
}
void SimplifyBooleanExprCheck::replaceWithThenStatement(
- const MatchFinder::MatchResult &Result, const Expr *TrueConditionRemoved) {
+ const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
- issueDiag(Result, TrueConditionRemoved->getBeginLoc(),
- SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+ IfStatement->getSourceRange(),
getText(Result, *IfStatement->getThen()));
}
void SimplifyBooleanExprCheck::replaceWithElseStatement(
- const MatchFinder::MatchResult &Result, const Expr *FalseConditionRemoved) {
+ const MatchFinder::MatchResult &Result, const Expr *BoolLiteral) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
const Stmt *ElseStatement = IfStatement->getElse();
- issueDiag(Result, FalseConditionRemoved->getBeginLoc(),
- SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ issueDiag(Result, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
+ IfStatement->getSourceRange(),
ElseStatement ? getText(Result, *ElseStatement) : "");
}
@@ -627,13 +709,7 @@
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 of a single
- // `return` statement returning the opposite boolean literal `true` or
- // `false`.
- assert(Compound->size() >= 2);
+ // Scan through the CompoundStmt to look for a chained-if construct.
const IfStmt *BeforeIf = nullptr;
CompoundStmt::const_body_iterator Current = Compound->body_begin();
CompoundStmt::const_body_iterator After = Compound->body_begin();
@@ -645,9 +721,8 @@
if (!ChainedConditionalReturn && BeforeIf)
continue;
- const Expr *Condition = If->getCond();
std::string Replacement =
- "return " + replacementExpression(Result, Negated, Condition);
+ "return " + replacementExpression(Result, Negated, If->getCond());
issueDiag(
Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
@@ -662,6 +737,38 @@
}
}
+void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated, const IfStmt *If) {
+ const auto *Lit = stmtReturnsBool(If, Negated);
+ const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
+ const std::string Replacement =
+ "return " + replacementExpression(Result, Negated, If->getCond());
+ issueDiag(Result, Lit->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+}
+
+void SimplifyBooleanExprCheck::replaceCaseCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const auto *CaseDefault = Result.Nodes.getNodeAs<CaseStmt>(CaseId);
+ const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
+void SimplifyBooleanExprCheck::replaceDefaultCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const SwitchCase *CaseDefault =
+ Result.Nodes.getNodeAs<DefaultStmt>(DefaultId);
+ const auto *If = dyn_cast<IfStmt>(CaseDefault->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
+void SimplifyBooleanExprCheck::replaceLabelCompoundReturnWithCondition(
+ const MatchFinder::MatchResult &Result, bool Negated) {
+ const auto *Label = Result.Nodes.getNodeAs<LabelStmt>(LabelId);
+ const auto *If = dyn_cast<IfStmt>(Label->getSubStmt());
+ replaceCompoundReturnWithCondition(Result, Negated, If);
+}
+
void SimplifyBooleanExprCheck::replaceWithAssignment(
const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
bool Negated) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
