[clang-tools-extra] 462d4de - [clangd] Add CMake option to (not) link in clang-tidy checks
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 14 01:12:29 PDT 2021
Author: Sam McCall
Date: 2021-07-14T10:04:21+02:00
New Revision: 462d4de35b0c9ef7157e49e147fc448a40c829b1
URL: https://github.com/llvm/llvm-project/commit/462d4de35b0c9ef7157e49e147fc448a40c829b1
DIFF: https://github.com/llvm/llvm-project/commit/462d4de35b0c9ef7157e49e147fc448a40c829b1.diff
LOG: [clangd] Add CMake option to (not) link in clang-tidy checks
This reduces the size of the dependency graph and makes incremental
development a little more pleasant (less rebuilding).
This introduces a bit of complexity/fragility as some tests verify
clang-tidy behavior. I attempted to isolate these and build/run as much
of the tests as possible in both configs to prevent rot.
Expectation is that (some) developers will use this locally, but
buildbots etc will keep testing clang-tidy.
Fixes https://github.com/clangd/clangd/issues/233
Differential Revision: https://reviews.llvm.org/D105679
Added:
clang-tools-extra/clangd/test/diagnostics-tidy.test
Modified:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/Features.cpp
clang-tools-extra/clangd/Features.inc.in
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/test/lit.cfg.py
clang-tools-extra/clangd/test/lit.site.cfg.py.in
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Removed:
clang-tools-extra/clangd/test/diagnostics.test
################################################################################
diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt
index 3c2b097e89fd1..5aee128c9d6b5 100644
--- a/clang-tools-extra/clangd/CMakeLists.txt
+++ b/clang-tools-extra/clangd/CMakeLists.txt
@@ -19,12 +19,15 @@ if (NOT DEFINED CLANGD_BUILD_XPC)
endif ()
option(CLANGD_MALLOC_TRIM "Call malloc_trim(3) periodically in Clangd. (only takes effect when using glibc)" ON)
+# -DCLANG_TIDY_CHECKS=Off avoids a dependency on clang-tidy, reducing rebuilds.
+option(CLANGD_TIDY_CHECKS "Link all clang-tidy checks into clangd" ON)
llvm_canonicalize_cmake_booleans(
CLANGD_BUILD_XPC
CLANGD_ENABLE_REMOTE
ENABLE_GRPC_REFLECTION
CLANGD_MALLOC_TRIM
+ CLANGD_TIDY_CHECKS
LLVM_ENABLE_ZLIB
)
@@ -162,10 +165,12 @@ target_link_libraries(clangDaemon
${LLVM_PTHREAD_LIB}
clangTidy
- ${ALL_CLANG_TIDY_CHECKS}
clangdSupport
)
+if(CLANGD_TIDY_CHECKS)
+ target_link_libraries(clangDaemon PRIVATE ${ALL_CLANG_TIDY_CHECKS})
+endif()
add_subdirectory(refactor/tweaks)
if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
diff --git a/clang-tools-extra/clangd/Features.cpp b/clang-tools-extra/clangd/Features.cpp
index 17f475fc4c22b..4ec03ea84bfb4 100644
--- a/clang-tools-extra/clangd/Features.cpp
+++ b/clang-tools-extra/clangd/Features.cpp
@@ -48,6 +48,10 @@ std::string featureString() {
#if CLANGD_BUILD_XPC
"+xpc"
#endif
+
+#if !CLANGD_TIDY_CHECKS
+ "-tidy"
+#endif
;
}
diff --git a/clang-tools-extra/clangd/Features.inc.in b/clang-tools-extra/clangd/Features.inc.in
index 72464d89b830e..b6c3729b0bb8d 100644
--- a/clang-tools-extra/clangd/Features.inc.in
+++ b/clang-tools-extra/clangd/Features.inc.in
@@ -3,3 +3,4 @@
#define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMOTE@
#define ENABLE_GRPC_REFLECTION @ENABLE_GRPC_REFLECTION@
#define CLANGD_MALLOC_TRIM @CLANGD_MALLOC_TRIM@
+#define CLANGD_TIDY_CHECKS @CLANGD_TIDY_CHECKS@
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index cb8ad5a8fa9ff..2520062eed9b5 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -15,6 +15,7 @@
#include "Config.h"
#include "Diagnostics.h"
#include "FeatureModule.h"
+#include "Features.h"
#include "Headers.h"
#include "HeuristicResolver.h"
#include "IncludeFixer.h"
@@ -60,8 +61,10 @@
// Force the linker to link in Clang-tidy modules.
// clangd doesn't support the static analyzer.
+#if CLANGD_TIDY_CHECKS
#define CLANG_TIDY_DISABLE_STATIC_ANALYZER_CHECKS
#include "../clang-tidy/ClangTidyForceLinker.h"
+#endif
namespace clang {
namespace clangd {
diff --git a/clang-tools-extra/clangd/test/diagnostics.test b/clang-tools-extra/clangd/test/diagnostics-tidy.test
similarity index 69%
rename from clang-tools-extra/clangd/test/diagnostics.test
rename to clang-tools-extra/clangd/test/diagnostics-tidy.test
index 588fefdbf2e06..1d10541be8bf0 100644
--- a/clang-tools-extra/clangd/test/diagnostics.test
+++ b/clang-tools-extra/clangd/test/diagnostics-tidy.test
@@ -1,27 +1,12 @@
+# REQUIRES: clangd-tidy-checks
# RUN: clangd -lit-test -clang-tidy-checks=bugprone-sizeof-expression < %s | FileCheck -strict-whitespace %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"void main() {\n(void)sizeof(42);\n}"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main() {\n(void)sizeof(42);\n}"}}}
# CHECK: "method": "textDocument/publishDiagnostics",
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
-# CHECK-NEXT: "code": "-Wmain-return-type",
-# CHECK-NEXT: "message": "Return type of 'main' is not 'int' (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 4,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 0,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
# CHECK-NEXT: "code": "bugprone-sizeof-expression",
# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'?",
# CHECK-NEXT: "range": {
@@ -54,3 +39,4 @@
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
+
diff --git a/clang-tools-extra/clangd/test/lit.cfg.py b/clang-tools-extra/clangd/test/lit.cfg.py
index 2680eb441df98..0f3d8b310b290 100644
--- a/clang-tools-extra/clangd/test/lit.cfg.py
+++ b/clang-tools-extra/clangd/test/lit.cfg.py
@@ -31,5 +31,8 @@ def calculate_arch_features(arch_string):
if config.clangd_enable_remote:
config.available_features.add('clangd-remote-index')
+if config.clangd_tidy_checks:
+ config.available_features.add('clangd-tidy-checks')
+
if config.have_zlib:
config.available_features.add('zlib')
diff --git a/clang-tools-extra/clangd/test/lit.site.cfg.py.in b/clang-tools-extra/clangd/test/lit.site.cfg.py.in
index efcc770095b36..853a3bf24a072 100644
--- a/clang-tools-extra/clangd/test/lit.site.cfg.py.in
+++ b/clang-tools-extra/clangd/test/lit.site.cfg.py.in
@@ -25,6 +25,7 @@ config.clangd_source_dir = "@CMAKE_CURRENT_SOURCE_DIR@/.."
config.clangd_binary_dir = "@CMAKE_CURRENT_BINARY_DIR@/.."
config.clangd_build_xpc = @CLANGD_BUILD_XPC@
config.clangd_enable_remote = @CLANGD_ENABLE_REMOTE@
+config.clangd_tidy_checks = @CLANGD_TIDY_CHECKS@
config.have_zlib = @LLVM_ENABLE_ZLIB@
# Delegate logic to lit.cfg.py.
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 93b85d8c0b5dc..eab28bb7fa0e5 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -288,16 +288,26 @@ TEST_F(ConfigCompileTests, Tidy) {
Tidy.CheckOptions.emplace_back(std::make_pair(
std::string("example-check.ExampleOption"), std::string("0")));
EXPECT_TRUE(compileAndApply());
- EXPECT_EQ(
- Conf.Diagnostics.ClangTidy.Checks,
- "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.size(), 2U);
EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.lookup("StrictMode"),
"true");
EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.lookup(
"example-check.ExampleOption"),
"0");
+#if CLANGD_TIDY_CHECKS
+ EXPECT_EQ(
+ Conf.Diagnostics.ClangTidy.Checks,
+ "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+#else // !CLANGD_TIDY_CHECKS
+ EXPECT_EQ(Conf.Diagnostics.ClangTidy.Checks, "llvm-*,-readability-*");
+ EXPECT_THAT(
+ Diags.Diagnostics,
+ ElementsAre(
+ DiagMessage(
+ "clang-tidy check 'bugprone-use-after-move' was not found"),
+ DiagMessage("clang-tidy check 'llvm-include-order' was not found")));
+#endif
}
TEST_F(ConfigCompileTests, TidyBadChecks) {
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 87f3c8737d464..1b04257df0579 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -10,6 +10,7 @@
#include "Config.h"
#include "Diagnostics.h"
#include "FeatureModule.h"
+#include "Features.h"
#include "ParsedAST.h"
#include "Protocol.h"
#include "SourceCode.h"
@@ -114,6 +115,18 @@ Position pos(int line, int character) {
return Res;
}
+// Normally returns the provided diagnostics matcher.
+// If clang-tidy checks are not linked in, returns a matcher for no diagnostics!
+// This is intended for tests where the diagnostics come from clang-tidy checks.
+// We don't #ifdef each individual test as it's intrusive and we want to ensure
+// that as much of the test is still compiled an run as possible.
+::testing::Matcher<std::vector<clangd::Diag>>
+ifTidyChecks(::testing::Matcher<std::vector<clangd::Diag>> M) {
+ if (!CLANGD_TIDY_CHECKS)
+ return IsEmpty();
+ return M;
+}
+
TEST(DiagnosticsTest, DiagnosticRanges) {
// Check we report correct ranges, including various edge-cases.
Annotations Test(R"cpp(
@@ -121,8 +134,11 @@ TEST(DiagnosticsTest, DiagnosticRanges) {
#define ID(X) X
namespace test{};
void $decl[[foo]]();
- class T{$explicit[[]]$constructor[[T]](int a);};
int main() {
+ struct Container { int* begin(); int* end(); } *container;
+ for (auto i : $insertstar[[]]$range[[container]]) {
+ }
+
$typo[[go\
o]]();
foo()$semicolon[[]]//with comments
@@ -135,10 +151,14 @@ o]]();
}
)cpp");
auto TU = TestTU::withCode(Test.code());
- TU.ClangTidyProvider = addTidyChecks("google-explicit-constructor");
EXPECT_THAT(
*TU.build().getDiagnostics(),
ElementsAre(
+ // Make sure the whole token is highlighted.
+ AllOf(Diag(Test.range("range"),
+ "invalid range expression of type 'struct Container *'; "
+ "did you mean to dereference it with '*'?"),
+ WithFix(Fix(Test.range("insertstar"), "*", "insert '*'"))),
// This range spans lines.
AllOf(Diag(Test.range("typo"),
"use of undeclared identifier 'goo'; did you mean 'foo'?"),
@@ -163,13 +183,7 @@ o]]();
AllOf(Diag(Test.range("macro"),
"use of undeclared identifier 'fod'; did you mean 'foo'?"),
WithFix(Fix(Test.range("macroarg"), "foo",
- "change 'fod' to 'foo'"))),
- // We make sure here that the entire token is highlighted
- AllOf(Diag(Test.range("constructor"),
- "single-argument constructors must be marked explicit to "
- "avoid unintentional implicit conversions"),
- WithFix(Fix(Test.range("explicit"), "explicit ",
- "insert 'explicit '")))));
+ "change 'fod' to 'foo'")))));
}
TEST(DiagnosticsTest, FlagsMatter) {
@@ -212,10 +226,10 @@ TEST(DiagnosticsTest, DeduplicatedClangTidyDiagnostics) {
// Verify that we filter out the duplicated diagnostic message.
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(::testing::AllOf(
+ ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
Diag(Test.range(),
"floating point literal has suffix 'f', which is not uppercase"),
- DiagSource(Diag::ClangTidy))));
+ DiagSource(Diag::ClangTidy)))));
Test = Annotations(R"cpp(
template<typename T>
@@ -232,10 +246,10 @@ TEST(DiagnosticsTest, DeduplicatedClangTidyDiagnostics) {
// duplicated messages, verify that we deduplicate them.
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(::testing::AllOf(
+ ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
Diag(Test.range(),
"floating point literal has suffix 'f', which is not uppercase"),
- DiagSource(Diag::ClangTidy))));
+ DiagSource(Diag::ClangTidy)))));
}
TEST(DiagnosticsTest, ClangTidy) {
@@ -265,9 +279,10 @@ TEST(DiagnosticsTest, ClangTidy) {
"modernize-deprecated-headers,"
"modernize-use-trailing-return-type,"
"misc-no-recursion");
+ TU.ExtraArgs.push_back("-Wno-unsequenced");
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(
+ ifTidyChecks(UnorderedElementsAre(
AllOf(Diag(Test.range("deprecated"),
"inclusion of deprecated C++ header 'assert.h'; consider "
"using 'cassert' instead"),
@@ -277,28 +292,25 @@ TEST(DiagnosticsTest, ClangTidy) {
"change '\"assert.h\"' to '<cassert>'"))),
Diag(Test.range("doubled"),
"suspicious usage of 'sizeof(sizeof(...))'"),
- AllOf(
- Diag(Test.range("macroarg"),
- "side effects in the 1st macro argument 'X' are repeated in "
- "macro expansion"),
- DiagSource(Diag::ClangTidy),
- DiagName("bugprone-macro-repeated-side-effects"),
- WithNote(
- Diag(Test.range("macrodef"), "macro 'SQUARE' defined here"))),
- Diag(Test.range("macroarg"),
- "multiple unsequenced modifications to 'y'"),
- AllOf(
- Diag(Test.range("main"),
- "use a trailing return type for this function"),
- DiagSource(Diag::ClangTidy),
- DiagName("modernize-use-trailing-return-type"),
- // Verify that we don't have "[check-name]" suffix in the message.
- WithFix(
- FixMessage("use a trailing return type for this function"))),
+ AllOf(Diag(Test.range("macroarg"),
+ "side effects in the 1st macro argument 'X' are "
+ "repeated in "
+ "macro expansion"),
+ DiagSource(Diag::ClangTidy),
+ DiagName("bugprone-macro-repeated-side-effects"),
+ WithNote(Diag(Test.range("macrodef"),
+ "macro 'SQUARE' defined here"))),
+ AllOf(Diag(Test.range("main"),
+ "use a trailing return type for this function"),
+ DiagSource(Diag::ClangTidy),
+ DiagName("modernize-use-trailing-return-type"),
+ // Verify there's no "[check-name]" suffix in the message.
+ WithFix(FixMessage(
+ "use a trailing return type for this function"))),
Diag(Test.range("foo"),
"function 'foo' is within a recursive call chain"),
Diag(Test.range("bar"),
- "function 'bar' is within a recursive call chain")));
+ "function 'bar' is within a recursive call chain"))));
}
TEST(DiagnosticsTest, ClangTidyEOF) {
@@ -313,9 +325,9 @@ TEST(DiagnosticsTest, ClangTidyEOF) {
TU.ClangTidyProvider = addTidyChecks("llvm-include-order");
EXPECT_THAT(
*TU.build().getDiagnostics(),
- Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"),
- DiagSource(Diag::ClangTidy),
- DiagName("llvm-include-order"))));
+ ifTidyChecks(Contains(
+ AllOf(Diag(Test.range(), "#includes are not sorted properly"),
+ DiagSource(Diag::ClangTidy), DiagName("llvm-include-order")))));
}
TEST(DiagnosticTest, TemplatesInHeaders) {
@@ -385,9 +397,9 @@ TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
TU.ClangTidyProvider = addTidyChecks("modernize-loop-convert");
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(::testing::AllOf(
+ ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
Diag(Main.range(), "use range-based for loop instead"),
- DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
+ DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert")))));
}
TEST(DiagnosticTest, RespectsDiagnosticConfig) {
@@ -430,10 +442,11 @@ TEST(DiagnosticTest, ClangTidySuppressionComment) {
TU.ClangTidyProvider = addTidyChecks("bugprone-integer-division");
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(::testing::AllOf(
+ ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
Diag(Main.range(), "result of integer division used in a floating "
"point context; possible loss of precision"),
- DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"))));
+ DiagSource(Diag::ClangTidy),
+ DiagName("bugprone-integer-division")))));
}
TEST(DiagnosticTest, ClangTidyWarningAsError) {
@@ -448,11 +461,11 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
addTidyChecks("bugprone-integer-division", "bugprone-integer-division");
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(::testing::AllOf(
+ ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
Diag(Main.range(), "result of integer division used in a floating "
"point context; possible loss of precision"),
DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
- DiagSeverity(DiagnosticsEngine::Error))));
+ DiagSeverity(DiagnosticsEngine::Error)))));
}
TEST(DiagnosticTest, LongFixMessages) {
@@ -528,9 +541,9 @@ TEST(DiagnosticTest, ElseAfterReturnRange) {
)cpp");
TestTU TU = TestTU::withCode(Main.code());
TU.ClangTidyProvider = addTidyChecks("llvm-else-after-return");
- EXPECT_THAT(
- *TU.build().getDiagnostics(),
- ElementsAre(Diag(Main.range(), "do not use 'else' after 'return'")));
+ EXPECT_THAT(*TU.build().getDiagnostics(),
+ ifTidyChecks(ElementsAre(
+ Diag(Main.range(), "do not use 'else' after 'return'"))));
}
TEST(DiagnosticsTest, Preprocessor) {
More information about the cfe-commits
mailing list