kadircet updated this revision to Diff 285389.
kadircet marked an inline comment as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83224/new/
https://reviews.llvm.org/D83224
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/ClangdTests.cpp
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -25,9 +25,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 "gmock/gmock.h"
#include "gtest/gtest.h"
#include <algorithm>
@@ -1166,6 +1168,44 @@
}
#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
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -808,23 +808,6 @@
// 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;
};
}
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ 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 @@
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() {
@@ -196,6 +233,15 @@
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.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
