njames93 updated this revision to Diff 304780. njames93 marked 4 inline comments as done. njames93 added a comment.
Reworked large chunks of this: - Renamed ClangdTidyProvider->TidyProvider. - Removed the obselete interface mirroring ClangTidyOptionsProvider, it wasn't needed. - Incorporated the ThreadsafeFS inside the provider. - Removed some unnecessary changes. - Removed the internal caching of the ThreadsafeFileTidyProvider. - Trimmed down inheritance hierarchy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/Compiler.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/TidyProvider.cpp clang-tools-extra/clangd/TidyProvider.h clang-tools-extra/clangd/tool/Check.cpp clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/CMakeLists.txt clang-tools-extra/clangd/unittests/ClangdTests.cpp clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang-tools-extra/clangd/unittests/TestTU.h clang-tools-extra/clangd/unittests/TestTidyProvider.cpp clang-tools-extra/clangd/unittests/TestTidyProvider.h
Index: clang-tools-extra/clangd/unittests/TestTidyProvider.h =================================================================== --- /dev/null +++ clang-tools-extra/clangd/unittests/TestTidyProvider.h @@ -0,0 +1,33 @@ +//===-- TestTidyProvider.h --------------------------------------*- C++ -*-===// +// +// 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 "../TidyProvider.h" +#include "llvm/Support/VirtualFileSystem.h" + +namespace clang { +namespace clangd { + +class TestClangTidyProvider : public TidyProvider { +public: + /// Convienence method for creating a provider that just uses \p Checks and \p + /// WarningsAsErrors + TestClangTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors = {}); + TestClangTidyProvider(const tidy::ClangTidyOptions &Options) + : Opts(Options) {} + + tidy::ClangTidyOptions getOptions(llvm::StringRef FileName) override { + return Opts; + } + +private: + tidy::ClangTidyOptions Opts; +}; + +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/unittests/TestTidyProvider.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/unittests/TestTidyProvider.cpp @@ -0,0 +1,26 @@ +//===-- TestTidyProvider.cpp ------------------------------------*- C++ -*-===// +// +// 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 "TestTidyProvider.h" + +namespace clang { + +using namespace tidy; + +namespace clangd { + +TestClangTidyProvider::TestClangTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors) { + if (!Checks.empty()) + Opts.Checks.emplace(Checks); + if (!WarningsAsErrors.empty()) + Opts.WarningsAsErrors.emplace(WarningsAsErrors); +} + +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/unittests/TestTU.h =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.h +++ clang-tools-extra/clangd/unittests/TestTU.h @@ -17,6 +17,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H +#include "../TidyProvider.h" #include "Compiler.h" #include "ParsedAST.h" #include "TestFS.h" @@ -58,8 +59,7 @@ // Extra arguments for the compiler invocation. std::vector<std::string> ExtraArgs; - llvm::Optional<std::string> ClangTidyChecks; - llvm::Optional<std::string> ClangTidyWarningsAsErrors; + std::unique_ptr<TidyProvider> ClangTidyProvider; // Index to use when building AST. const SymbolIndex *ExternalIndex = nullptr; Index: clang-tools-extra/clangd/unittests/TestTU.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.cpp +++ clang-tools-extra/clangd/unittests/TestTU.cpp @@ -59,8 +59,7 @@ FS.OverlayRealFileSystemForModules = true; Inputs.TFS = &FS; Inputs.Opts = ParseOptions(); - Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; - Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors; + Inputs.ClangTidyProvider = ClangTidyProvider.get(); Inputs.Index = ExternalIndex; if (Inputs.Index) Inputs.Opts.SuggestMissingIncludes = true; Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -24,6 +24,7 @@ #include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" +#include "TestTidyProvider.h" #include "clang/AST/DeclTemplate.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" @@ -250,7 +251,8 @@ TestTU TU; // this check runs the preprocessor, we need to make sure it does not break // our recording logic. - TU.ClangTidyChecks = "modernize-use-trailing-return-type"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "modernize-use-trailing-return-type"); TU.Code = "inline int foo() {}"; auto AST = TU.build(); @@ -406,7 +408,8 @@ "replay-preamble-module", ""); TestTU TU; // This check records inclusion directives replayed by clangd. - TU.ClangTidyChecks = "replay-preamble-check"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("replay-preamble-check"); llvm::Annotations Test(R"cpp( $hash^#$include[[import]] $filebegin^"$filerange[[bar.h]]" $hash^#$include[[include_next]] $filebegin^"$filerange[[baz.h]]" Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -14,6 +14,7 @@ #include "TestFS.h" #include "TestIndex.h" #include "TestTU.h" +#include "TestTidyProvider.h" #include "index/MemIndex.h" #include "support/Path.h" #include "clang/Basic/Diagnostic.h" @@ -129,7 +130,8 @@ } )cpp"); auto TU = TestTU::withCode(Test.code()); - TU.ClangTidyChecks = "-*,google-explicit-constructor"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("-*,google-explicit-constructor"); EXPECT_THAT( TU.build().getDiagnostics(), ElementsAre( @@ -201,8 +203,9 @@ auto TU = TestTU::withCode(Test.code()); // Enable alias clang-tidy checks, these check emits the same diagnostics // (except the check name). - TU.ClangTidyChecks = "-*, readability-uppercase-literal-suffix, " - "hicpp-uppercase-literal-suffix"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "-*, readability-uppercase-literal-suffix, " + "hicpp-uppercase-literal-suffix"); // Verify that we filter out the duplicated diagnostic message. EXPECT_THAT( TU.build().getDiagnostics(), @@ -245,9 +248,9 @@ )cpp"); auto TU = TestTU::withCode(Test.code()); TU.HeaderFilename = "assert.h"; // Suppress "not found" error. - TU.ClangTidyChecks = + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, " - "modernize-deprecated-headers, modernize-use-trailing-return-type"; + "modernize-deprecated-headers, modernize-use-trailing-return-type"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre( @@ -289,7 +292,8 @@ auto TU = TestTU::withCode(Test.code()); TU.ExtraArgs = {"-isystem."}; TU.AdditionalFiles["a.h"] = TU.AdditionalFiles["b.h"] = ""; - TU.ClangTidyChecks = "-*, llvm-include-order"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("-*, llvm-include-order"); EXPECT_THAT( TU.build().getDiagnostics(), Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"), @@ -361,7 +365,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "modernize-loop-convert"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("modernize-loop-convert"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -384,7 +389,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -401,8 +407,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "bugprone-integer-division", "bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -452,8 +458,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "bugprone-integer-division", "bugprone-integer-division"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); } @@ -468,7 +474,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "-*,bugprone-bad-signal-to-kill-thread"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "-*,bugprone-bad-signal-to-kill-thread"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash } Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -16,6 +16,7 @@ #include "SyncAPI.h" #include "TestFS.h" #include "TestTU.h" +#include "TidyProvider.h" #include "URI.h" #include "support/MemoryTree.h" #include "support/Path.h" @@ -1214,17 +1215,21 @@ bool HadDiagsInLastCallback = false; } DiagConsumer; + // We need to use the actual options provider here as we're interested in + // making sure those providers correctly disable problematic checks. + // As an added bonus it demonstrates the provider is capable of handling the + // ThreadsafeFS. + MockFS FS; + FS.Files[testPath(".clang-tidy")] = R"( + Checks: -*,bugprone-use-after-move,llvm-header-guard + )"; MockCompilationDatabase CDB; + ThreadSafeFileTidyProvider Provider(FS, + tidy::ClangTidyOptions::getDefaults()); 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; - }; + Opts.ClangTidyProvider = &Provider; ClangdServer Server(CDB, FS, Opts, &DiagConsumer); const char *SourceContents = R"cpp( struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); }; Index: clang-tools-extra/clangd/unittests/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/unittests/CMakeLists.txt +++ clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -87,6 +87,7 @@ TUSchedulerTests.cpp TestFS.cpp TestIndex.cpp + TestTidyProvider.cpp TestTU.cpp TestWorkspace.cpp TypeHierarchyTests.cpp Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -11,6 +11,7 @@ #include "Features.inc" #include "PathMapping.h" #include "Protocol.h" +#include "TidyProvider.h" #include "Transport.h" #include "index/Background.h" #include "index/Serialization.h" @@ -803,9 +804,7 @@ } // Create an empty clang-tidy option. - std::mutex ClangTidyOptMu; - std::unique_ptr<tidy::ClangTidyOptionsProvider> - ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/ + std::unique_ptr<TidyProvider> ClangTidyOptProvider; if (EnableClangTidy) { auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults(); EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set. @@ -814,24 +813,13 @@ if (!EmptyDefaults.User) EmptyDefaults.User = llvm::sys::Process::GetEnv("USERNAME"); #endif - tidy::ClangTidyOptions OverrideClangTidyOptions; - if (!ClangTidyChecks.empty()) - OverrideClangTidyOptions.Checks = ClangTidyChecks; - ClangTidyOptProvider = std::make_unique<tidy::FileOptionsProvider>( - tidy::ClangTidyGlobalOptions(), - /* Default */ EmptyDefaults, - /* Override */ OverrideClangTidyOptions, TFS.view(/*CWD=*/llvm::None)); - Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &, - llvm::StringRef File) { - // This function must be thread-safe and tidy option providers are not. - tidy::ClangTidyOptions Opts; - { - std::lock_guard<std::mutex> Lock(ClangTidyOptMu); - // FIXME: use the FS provided to the function. - Opts = ClangTidyOptProvider->getOptions(File); - } - return Opts; - }; + if (EnableConfig) + ClangTidyOptProvider = std::make_unique<ConfigTidyProvider>( + TFS, std::move(EmptyDefaults), ClangTidyChecks); + else + ClangTidyOptProvider = std::make_unique<ThreadSafeFileTidyProvider>( + TFS, std::move(EmptyDefaults), ClangTidyChecks); + Opts.ClangTidyProvider = ClangTidyOptProvider.get(); } Opts.AsyncPreambleBuilds = AsyncPreamble; Opts.SuggestMissingIncludes = SuggestMissingIncludes; Index: clang-tools-extra/clangd/tool/Check.cpp =================================================================== --- clang-tools-extra/clangd/tool/Check.cpp +++ clang-tools-extra/clangd/tool/Check.cpp @@ -133,8 +133,6 @@ return false; } } - Inputs.Opts.ClangTidyOpts = - Opts.GetClangTidyOptions(*TFS.view(llvm::None), File); log("Parsing command..."); Invocation = buildCompilerInvocation(Inputs, CaptureInvocationDiags, &CC1Args); Index: clang-tools-extra/clangd/TidyProvider.h =================================================================== --- /dev/null +++ clang-tools-extra/clangd/TidyProvider.h @@ -0,0 +1,65 @@ +//===--- TidyProvider.h - create options for running 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H + +#include "../clang-tidy/ClangTidyOptions.h" +#include "support/ThreadsafeFS.h" +#include "llvm/ADT/StringRef.h" + +namespace clang { +namespace clangd { + +/// The base class of all clang-tidy config providers for clangd. +class TidyProvider { +public: + /// Returns a provider than can be passed directly to the constructor of \ref + /// ClangTidyContext which delegates its option sourcing to this class. + virtual std::unique_ptr<tidy::ClangTidyOptionsProvider> getInstance(); + + virtual tidy::ClangTidyOptions getOptions(llvm::StringRef FileName) = 0; + + virtual ~TidyProvider() = default; +}; + +/// A TidyProvider that uses a Threadsafe Filesystem to traverse up the +/// directory of a file looking for .clang-tidy configuration files. +class ThreadSafeFileTidyProvider : public TidyProvider { +public: + ThreadSafeFileTidyProvider(ThreadsafeFS &TFS, + tidy::ClangTidyOptions DefaultOptions, + llvm::StringRef OverrideChecks = {}) + : TFS(TFS), DefaultOpts(std::move(DefaultOptions)), + OverrideChecks(OverrideChecks) {} + + tidy::ClangTidyOptions getOptions(llvm::StringRef FileName) override; + +protected: + tidy::ClangTidyOptions getOptionsNoOverride(llvm::StringRef FileName); + void addOverrideChecks(tidy::ClangTidyOptions &Opts); + +private: + ThreadsafeFS &TFS; + const tidy::ClangTidyOptions DefaultOpts; + const std::string OverrideChecks; +}; + +/// A ThreadSafeFileTidyProvider that will append options found in the clangd +/// configuration to those found in .clang-tidy configuration files. +class ConfigTidyProvider : public ThreadSafeFileTidyProvider { +public: + using ThreadSafeFileTidyProvider::ThreadSafeFileTidyProvider; + + tidy::ClangTidyOptions getOptions(llvm::StringRef FileName) override; +}; + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H Index: clang-tools-extra/clangd/TidyProvider.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/TidyProvider.cpp @@ -0,0 +1,192 @@ +//===--- TidyProvider.cpp - create options for running 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 "TidyProvider.h" +#include "Config.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/Support/VirtualFileSystem.h" + +namespace clang { +namespace clangd { + +static void mergeCheckList(llvm::Optional<std::string> &Checks, + llvm::StringRef List) { + if (List.empty()) + return; + if (!Checks || Checks->empty()) { + Checks.emplace(List); + return; + } + Checks->append(("," + List).str()); +} + +namespace { + +/// An instance of ClangTidyOptionsProvider that can be used when constructing a +/// \ref ClangTidyContext. It delegates sourcing options to a TidyProvider. If +/// no checks are specified, it uses a nice default set of checks, otherwise it +/// removes any checks known to cause issue in clangd. +class CheckAdjustingProxyTidyProvider : public tidy::ClangTidyOptionsProvider { +public: + CheckAdjustingProxyTidyProvider(TidyProvider &Provider) + : Provider(Provider) {} + + const tidy::ClangTidyGlobalOptions &getGlobalOptions() override { + return GlobalOpt; + } + std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override { + tidy::ClangTidyOptions Opts = Provider.getOptions(FileName); + if (Opts.Checks && !Opts.Checks->empty()) + Opts.Checks->append(("," + getUnusableTidyChecks()).str()); + else + Opts.Checks.emplace(getDefaultTidyChecks()); + // We dont care about the source of the options, as clangd never uses it. + return {1, OptionsSource(std::move(Opts), "")}; + } + +private: + static 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; + } + + static 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; + } + + TidyProvider &Provider; + tidy::ClangTidyGlobalOptions GlobalOpt = {}; +}; + +} // namespace + +std::unique_ptr<tidy::ClangTidyOptionsProvider> TidyProvider::getInstance() { + return std::make_unique<CheckAdjustingProxyTidyProvider>(*this); +} + +static llvm::Optional<tidy::ClangTidyOptions> +tryReadConfigFile(llvm::vfs::FileSystem *FS, llvm::StringRef Directory) { + assert(!Directory.empty()); + + llvm::ErrorOr<llvm::vfs::Status> DirectoryStatus = FS->status(Directory); + + if (!DirectoryStatus || !DirectoryStatus->isDirectory()) { + llvm::errs() << "Error reading configuration from " << Directory + << ": directory doesn't exist.\n"; + return llvm::None; + } + + llvm::SmallString<128> ConfigFile(Directory); + llvm::sys::path::append(ConfigFile, ".clang-tidy"); + + llvm::ErrorOr<llvm::vfs::Status> FileStatus = FS->status(ConfigFile); + + if (!FileStatus || !FileStatus->isRegularFile()) + return llvm::None; + + llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text = + FS->getBufferForFile(ConfigFile); + if (std::error_code EC = Text.getError()) { + llvm::errs() << "Can't read " << ConfigFile << ": " << EC.message() << "\n"; + return llvm::None; + } + + // Skip empty files, e.g. files opened for writing via shell output + // redirection. + if ((*Text)->getBuffer().empty()) + return llvm::None; + llvm::ErrorOr<tidy::ClangTidyOptions> ParsedOptions = + tidy::parseConfiguration((*Text)->getBuffer()); + if (!ParsedOptions) { + if (ParsedOptions.getError()) + llvm::errs() << "Error parsing " << ConfigFile << ": " + << ParsedOptions.getError().message() << "\n"; + return llvm::None; + } + return std::move(*ParsedOptions); +} + +tidy::ClangTidyOptions +ThreadSafeFileTidyProvider::getOptions(llvm::StringRef FileName) { + auto Options = getOptionsNoOverride(FileName); + addOverrideChecks(Options); + return Options; +} + +tidy::ClangTidyOptions +ThreadSafeFileTidyProvider::getOptionsNoOverride(llvm::StringRef FileName) { + tidy::ClangTidyOptions Options = DefaultOpts; + llvm::SmallVector<tidy::ClangTidyOptions, 4> OptionStack; + auto FS(TFS.view(llvm::None)); + llvm::SmallString<128> AbsolutePath(FileName); + + if (FS->makeAbsolute(AbsolutePath)) + return Options; + + llvm::StringRef Path = llvm::sys::path::parent_path(AbsolutePath); + for (llvm::StringRef CurrentPath = Path; !CurrentPath.empty(); + CurrentPath = llvm::sys::path::parent_path(CurrentPath)) { + auto ConfigFile = tryReadConfigFile(FS.get(), CurrentPath); + if (!ConfigFile) + continue; + OptionStack.push_back(std::move(*ConfigFile)); + if (!OptionStack.back().InheritParentConfig.getValueOr(false)) + break; + } + unsigned Priority = 0U; + for (auto &Option : llvm::reverse(OptionStack)) { + Options = Options.mergeWith(Option, ++Priority); + } + return Options; +} +void ThreadSafeFileTidyProvider::addOverrideChecks( + tidy::ClangTidyOptions &Opts) { + mergeCheckList(Opts.Checks, OverrideChecks); +} + +tidy::ClangTidyOptions +ConfigTidyProvider::getOptions(llvm::StringRef FileName) { + tidy::ClangTidyOptions Current = + ThreadSafeFileTidyProvider::getOptionsNoOverride(FileName); + const auto &CurTidyConfig = Config::current().ClangTidy; + + if (!CurTidyConfig.Checks.empty()) + mergeCheckList(Current.Checks, CurTidyConfig.Checks); + + for (const auto &CheckOption : CurTidyConfig.CheckOptions) + Current.CheckOptions.insert_or_assign( + CheckOption.getKey(), + tidy::ClangTidyOptions::ClangTidyValue(CheckOption.getValue())); + addOverrideChecks(Current); + return Current; +} +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/ParsedAST.cpp =================================================================== --- clang-tools-extra/clangd/ParsedAST.cpp +++ clang-tools-extra/clangd/ParsedAST.cpp @@ -17,6 +17,7 @@ #include "IncludeFixer.h" #include "Preamble.h" #include "SourceCode.h" +#include "TidyProvider.h" #include "index/CanonicalIncludes.h" #include "index/Index.h" #include "support/Logger.h" @@ -292,16 +293,22 @@ llvm::Optional<tidy::ClangTidyContext> CTContext; { trace::Span Tracer("ClangTidyInit"); - dlog("ClangTidy configuration for file {0}: {1}", Filename, - tidy::configurationAsText(Inputs.Opts.ClangTidyOpts)); tidy::ClangTidyCheckFactories CTFactories; for (const auto &E : tidy::ClangTidyModuleRegistry::entries()) E.instantiate()->addCheckFactories(CTFactories); - CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>( - tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts)); + if (Inputs.ClangTidyProvider) { + CTContext.emplace(Inputs.ClangTidyProvider->getInstance()); + } else { + // This OptionsProvider will have no checks enabled, essentially disabling + // clang-tidy. + CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>( + tidy::ClangTidyGlobalOptions(), tidy::ClangTidyOptions())); + } CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); CTContext->setASTContext(&Clang->getASTContext()); CTContext->setCurrentFile(Filename); + dlog("ClangTidy configuration for file {0}: {1}", Filename, + tidy::configurationAsText(CTContext->getOptions())); CTChecks = CTFactories.createChecks(CTContext.getPointer()); llvm::erase_if(CTChecks, [&](const auto &Check) { return !Check->isLanguageVersionSupported(CTContext->getLangOpts()); Index: clang-tools-extra/clangd/Compiler.h =================================================================== --- clang-tools-extra/clangd/Compiler.h +++ clang-tools-extra/clangd/Compiler.h @@ -15,7 +15,6 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H -#include "../clang-tidy/ClangTidyOptions.h" #include "GlobalCompilationDatabase.h" #include "index/Index.h" #include "support/ThreadsafeFS.h" @@ -26,6 +25,8 @@ namespace clang { namespace clangd { +class TidyProvider; + class IgnoreDiagnostics : public DiagnosticConsumer { public: static void log(DiagnosticsEngine::Level DiagLevel, @@ -37,7 +38,6 @@ // Options to run clang e.g. when parsing AST. struct ParseOptions { - tidy::ClangTidyOptions ClangTidyOpts; bool SuggestMissingIncludes = false; bool BuildRecoveryAST = false; bool PreserveRecoveryASTType = false; @@ -56,6 +56,7 @@ // Used to recover from diagnostics (e.g. find missing includes for symbol). const SymbolIndex *Index = nullptr; ParseOptions Opts = ParseOptions(); + TidyProvider *ClangTidyProvider = nullptr; }; /// Builds compiler invocation that could be used to build AST or preamble. Index: clang-tools-extra/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/clangd/ClangdServer.h +++ clang-tools-extra/clangd/ClangdServer.h @@ -41,13 +41,7 @@ namespace clang { namespace clangd { - -/// When set, used by ClangdServer to get clang-tidy options for each particular -/// file. Must be thread-safe. We use this instead of ClangTidyOptionsProvider -/// to allow reading tidy configs from the VFS used for parsing. -using ClangTidyOptionsBuilder = std::function<tidy::ClangTidyOptions( - llvm::vfs::FileSystem &, llvm::StringRef /*File*/)>; - +class TidyProvider; /// Manages a collection of source files and derived data (ASTs, indexes), /// and provides language-aware features such as code completion. /// @@ -121,12 +115,9 @@ /// If set, queried to obtain the configuration to handle each request. config::Provider *ConfigProvider = nullptr; - /// If set, enable clang-tidy in clangd and use to it get clang-tidy - /// configurations for a particular file. - /// Clangd supports only a small subset of ClangTidyOptions, these options - /// (Checks, CheckOptions) are about which clang-tidy checks will be - /// enabled. - ClangTidyOptionsBuilder GetClangTidyOptions; + /// The Options provider to use when running clang-tidy. If null, clang-tidy + /// checks will be disabled. + TidyProvider *ClangTidyProvider = nullptr; /// If true, force -frecovery-ast flag. /// If false, respect the value in clang. @@ -374,7 +365,7 @@ std::vector<std::unique_ptr<SymbolIndex>> MergedIdx; // When set, provides clang-tidy options for a specific file. - ClangTidyOptionsBuilder GetClangTidyOptions; + TidyProvider *ClangTidyProvider = nullptr; // If this is true, suggest include insertion fixes for diagnostic errors that // can be caused by missing includes (e.g. member access in incomplete type). Index: clang-tools-extra/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdServer.cpp +++ clang-tools-extra/clangd/ClangdServer.cpp @@ -113,40 +113,6 @@ 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() { @@ -177,7 +143,7 @@ ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex, Opts.CollectMainFileRefs) : nullptr), - GetClangTidyOptions(Opts.GetClangTidyOptions), + ClangTidyProvider(Opts.ClangTidyProvider), SuggestMissingIncludes(Opts.SuggestMissingIncludes), BuildRecoveryAST(Opts.BuildRecoveryAST), PreserveRecoveryASTType(Opts.PreserveRecoveryASTType), @@ -235,20 +201,6 @@ llvm::StringRef Version, WantDiagnostics WantDiags, bool ForceRebuild) { ParseOptions Opts; - Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults(); - // FIXME: call tidy options builder on the worker thread, it can do IO. - 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. @@ -259,6 +211,7 @@ Inputs.ForceRebuild = ForceRebuild; Inputs.Opts = std::move(Opts); Inputs.Index = Index; + Inputs.ClangTidyProvider = ClangTidyProvider; Inputs.Opts.BuildRecoveryAST = BuildRecoveryAST; Inputs.Opts.PreserveRecoveryASTType = PreserveRecoveryASTType; bool NewFile = WorkScheduler.update(File, Inputs, WantDiags); Index: clang-tools-extra/clangd/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/CMakeLists.txt +++ clang-tools-extra/clangd/CMakeLists.txt @@ -77,6 +77,7 @@ SemanticSelection.cpp SourceCode.cpp QueryDriverDatabase.cpp + TidyProvider.cpp TUScheduler.cpp URI.cpp XRefs.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
