[clang-tools-extra] 0464acd - [clangd] Move clang-tidy check modifications into ClangdServer
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 13 09:33:47 PDT 2020
Author: Kadir Cetinkaya
Date: 2020-08-13T18:32:59+02:00
New Revision: 0464acd0197cda149f81dff20bf5c379a057722a
URL: https://github.com/llvm/llvm-project/commit/0464acd0197cda149f81dff20bf5c379a057722a
DIFF: https://github.com/llvm/llvm-project/commit/0464acd0197cda149f81dff20bf5c379a057722a.diff
LOG: [clangd] Move clang-tidy check modifications into ClangdServer
Summary:
This enables sharing the logic between standalone clangd and embedders
of it. The new approach should be same performance-wise, as it is only called
once per addDocument call.
This patch also introduces a blacklisting code path for disabling crashy or
high-noise tests, until we figure out a way to make them work with clangd-setup.
The biggest difference is the way we make use of preambles, hence those checks
can't see directives coming from the preamble section of the file. The second
thing is the fact that code might-not be compiling while clangd is trying to
build an AST, hence some checks might choke on those incomplete ASTs.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, aaron.ballman, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83224
Added:
Modified:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/ClangdTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 300d345f8eeb..74ab21a5f778 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -40,6 +40,7 @@
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
@@ -52,6 +53,7 @@
#include <future>
#include <memory>
#include <mutex>
+#include <string>
#include <type_traits>
namespace clang {
@@ -109,6 +111,41 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
ClangdServer::Callbacks *ServerCallbacks;
bool TheiaSemanticHighlighting;
};
+
+// Set of clang-tidy checks that don't work well in clangd, either due to
+// crashes or false positives.
+// Returns a clang-tidy filter string: -check1,-check2.
+llvm::StringRef getUnusableTidyChecks() {
+ static const std::string FalsePositives =
+ llvm::join_items(", ",
+ // Check relies on seeing ifndef/define/endif directives,
+ // clangd doesn't replay those when using a preamble.
+ "-llvm-header-guard");
+ static const std::string CrashingChecks =
+ llvm::join_items(", ",
+ // Check can choke on invalid (intermediate) c++ code,
+ // which is often the case when clangd tries to build an
+ // AST.
+ "-bugprone-use-after-move");
+ static const std::string UnusableTidyChecks =
+ llvm::join_items(", ", FalsePositives, CrashingChecks);
+ return UnusableTidyChecks;
+}
+
+// Returns a clang-tidy options string: check1,check2.
+llvm::StringRef getDefaultTidyChecks() {
+ // These default checks are chosen for:
+ // - low false-positive rate
+ // - providing a lot of value
+ // - being reasonably efficient
+ static const std::string DefaultChecks = llvm::join_items(
+ ",", "readability-misleading-indentation", "readability-deleted-default",
+ "bugprone-integer-division", "bugprone-sizeof-expression",
+ "bugprone-suspicious-missing-comma", "bugprone-unused-raii",
+ "bugprone-unused-return-value", "misc-unused-using-decls",
+ "misc-unused-alias-decls", "misc-definitions-in-headers");
+ return DefaultChecks;
+}
} // namespace
ClangdServer::Options ClangdServer::optsForTest() {
@@ -200,6 +237,15 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
if (GetClangTidyOptions)
Opts.ClangTidyOpts =
GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+ if (Opts.ClangTidyOpts.Checks.hasValue()) {
+ // If the set of checks was configured, make sure clangd incompatible ones
+ // are disabled.
+ Opts.ClangTidyOpts.Checks = llvm::join_items(
+ ", ", *Opts.ClangTidyOpts.Checks, getUnusableTidyChecks());
+ } else {
+ // Otherwise provide a nice set of defaults.
+ Opts.ClangTidyOpts.Checks = getDefaultTidyChecks().str();
+ }
Opts.SuggestMissingIncludes = SuggestMissingIncludes;
// Compile command is set asynchronously during update, as it can be slow.
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 484ece01b6c9..3d83f3652f30 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -808,23 +808,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
// FIXME: use the FS provided to the function.
Opts = ClangTidyOptProvider->getOptions(File);
}
- if (!Opts.Checks) {
- // If the user hasn't configured clang-tidy checks at all, including
- // via .clang-tidy, give them a nice set of checks.
- // (This should be what the "default" options does, but it isn't...)
- //
- // These default checks are chosen for:
- // - low false-positive rate
- // - providing a lot of value
- // - being reasonably efficient
- Opts.Checks = llvm::join_items(
- ",", "readability-misleading-indentation",
- "readability-deleted-default", "bugprone-integer-division",
- "bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
- "bugprone-unused-raii", "bugprone-unused-return-value",
- "misc-unused-using-decls", "misc-unused-alias-decls",
- "misc-definitions-in-headers");
- }
return Opts;
};
}
diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index 284b241b47fa..813b95aa3c82 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -26,9 +26,11 @@
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -1194,6 +1196,44 @@ TEST(ClangdServerTest, TestStackOverflow) {
}
#endif
+TEST(ClangdServer, TidyOverrideTest) {
+ struct DiagsCheckingCallback : public ClangdServer::Callbacks {
+ public:
+ void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+ std::vector<Diag> Diagnostics) override {
+ std::lock_guard<std::mutex> Lock(Mutex);
+ HadDiagsInLastCallback = !Diagnostics.empty();
+ }
+
+ std::mutex Mutex;
+ bool HadDiagsInLastCallback = false;
+ } DiagConsumer;
+
+ MockFS FS;
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags = {"-xc++"};
+ auto Opts = ClangdServer::optsForTest();
+ Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) {
+ auto Opts = tidy::ClangTidyOptions::getDefaults();
+ // These checks don't work well in clangd, even if configured they shouldn't
+ // run.
+ Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+ return Opts;
+ };
+ ClangdServer Server(CDB, FS, Opts, &DiagConsumer);
+ const char *SourceContents = R"cpp(
+ struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); };
+ namespace std { Foo&& move(Foo&); }
+ void foo() {
+ Foo x;
+ Foo y = std::move(x);
+ Foo z = x;
+ })cpp";
+ Server.addDocument(testPath("foo.h"), SourceContents);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
+ EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list