From ecc6965b2342397a215b00e8e476d8d37d080322 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Tue, 13 Apr 2021 10:26:03 +0200 Subject: [PATCH] Revert "Revert "[clangd] Provide a way to disable external index"" This reverts commit c2ad7c23707cece995ee9070283a72c4afc8c0fe while adding the handling for the new enum value into the switch statement. --- clang-tools-extra/clangd/Config.h | 2 +- clang-tools-extra/clangd/ConfigCompile.cpp | 38 ++++++++++++------- clang-tools-extra/clangd/ConfigFragment.h | 3 ++ clang-tools-extra/clangd/ConfigYAML.cpp | 24 +++++++++++- clang-tools-extra/clangd/tool/ClangdMain.cpp | 2 + .../clangd/unittests/ConfigCompileTests.cpp | 18 +++++++-- .../clangd/unittests/ConfigYAMLTests.cpp | 17 +++++++++ 7 files changed, 84 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h index 7064edd76b8f..fe6f4d7fa6e8 100644 --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -70,7 +70,7 @@ struct Config { enum class BackgroundPolicy { Build, Skip }; /// Describes an external index configuration. struct ExternalIndexSpec { - enum { File, Server } Kind; + enum { None, File, Server } Kind; /// This is one of: /// - Address of a clangd-index-server, in the form of "ip:port". /// - Absolute path to an index produced by clangd-indexer. diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp index 1185eb7255b4..a5745727dca7 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -329,10 +329,11 @@ struct FragmentCompiler { } #endif // Make sure exactly one of the Sources is set. - unsigned SourceCount = - External.File.hasValue() + External.Server.hasValue(); + unsigned SourceCount = External.File.hasValue() + + External.Server.hasValue() + *External.IsNone; if (SourceCount != 1) { - diag(Error, "Exactly one of File or Server must be set.", BlockRange); + diag(Error, "Exactly one of File, Server or None must be set.", + BlockRange); return; } Config::ExternalIndexSpec Spec; @@ -346,20 +347,29 @@ struct FragmentCompiler { if (!AbsPath) return; Spec.Location = std::move(*AbsPath); + } else { + assert(*External.IsNone); + Spec.Kind = Config::ExternalIndexSpec::None; } - // Make sure MountPoint is an absolute path with forward slashes. - if (!External.MountPoint) - External.MountPoint.emplace(FragmentDirectory); - if ((**External.MountPoint).empty()) { - diag(Error, "A mountpoint is required.", BlockRange); - return; + if (Spec.Kind != Config::ExternalIndexSpec::None) { + // Make sure MountPoint is an absolute path with forward slashes. + if (!External.MountPoint) + External.MountPoint.emplace(FragmentDirectory); + if ((**External.MountPoint).empty()) { + diag(Error, "A mountpoint is required.", BlockRange); + return; + } + auto AbsPath = makeAbsolute(std::move(*External.MountPoint), "MountPoint", + llvm::sys::path::Style::posix); + if (!AbsPath) + return; + Spec.MountPoint = std::move(*AbsPath); } - auto AbsPath = makeAbsolute(std::move(*External.MountPoint), "MountPoint", - llvm::sys::path::Style::posix); - if (!AbsPath) - return; - Spec.MountPoint = std::move(*AbsPath); Out.Apply.push_back([Spec(std::move(Spec))](const Params &P, Config &C) { + if (Spec.Kind == Config::ExternalIndexSpec::None) { + C.Index.External.reset(); + return; + } if (P.Path.empty() || !pathStartsWith(Spec.MountPoint, P.Path, llvm::sys::path::Style::posix)) return; diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h index 1365ed4c1037..6b58b88dfd49 100644 --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -173,6 +173,9 @@ struct Fragment { /// usually prepared using clangd-indexer. /// Exactly one source (File/Server) should be configured. struct ExternalBlock { + /// Whether the block is explicitly set to `None`. Can be used to clear + /// any external index specified before. + Located IsNone = false; /// Path to an index file generated by clangd-indexer. Relative paths may /// be used, if config fragment is associated with a directory. llvm::Optional> File; diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp index d50c01168a8d..f5739de0092d 100644 --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -12,6 +12,7 @@ #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/SourceMgr.h" #include "llvm/Support/YAMLParser.h" +#include #include namespace clang { @@ -149,13 +150,34 @@ private: [&](Node &N) { F.Background = scalarValue(N, "Background"); }); Dict.handle("External", [&](Node &N) { Fragment::IndexBlock::ExternalBlock External; - parse(External, N); + // External block can either be a mapping or a scalar value. Dispatch + // accordingly. + if (N.getType() == Node::NK_Mapping) { + parse(External, N); + } else if (N.getType() == Node::NK_Scalar || + N.getType() == Node::NK_BlockScalar) { + parse(External, scalarValue(N, "External").getValue()); + } else { + error("External must be either a scalar or a mapping.", N); + return; + } F.External.emplace(std::move(External)); F.External->Range = N.getSourceRange(); }); Dict.parse(N); } + void parse(Fragment::IndexBlock::ExternalBlock &F, + Located ExternalVal) { + if (!llvm::StringRef(*ExternalVal).equals_lower("none")) { + error("Only scalar value supported for External is 'None'", + ExternalVal.Range); + return; + } + F.IsNone = true; + F.IsNone.Range = ExternalVal.Range; + } + void parse(Fragment::IndexBlock::ExternalBlock &F, Node &N) { DictParser Dict("External", this); Dict.handle("File", [&](Node &N) { F.File = scalarValue(N, "File"); }); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index 6dd13a503887..77e8ff1539e9 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -551,6 +551,8 @@ std::unique_ptr loadExternalIndex(const Config::ExternalIndexSpec &External, AsyncTaskRunner *Tasks) { switch (External.Kind) { + case Config::ExternalIndexSpec::None: + break; case Config::ExternalIndexSpec::Server: log("Associating {0} with remote index at {1}.", External.MountPoint, External.Location); diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp index f2c27c22fdeb..23347e2324fe 100644 --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -327,21 +327,31 @@ TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) { #ifdef CLANGD_ENABLE_REMOTE EXPECT_THAT( Diags.Diagnostics, - Contains(AllOf(DiagMessage("Exactly one of File or Server must be set."), - DiagKind(llvm::SourceMgr::DK_Error)))); + Contains( + AllOf(DiagMessage("Exactly one of File, Server or None must be set."), + DiagKind(llvm::SourceMgr::DK_Error)))); #else ASSERT_TRUE(Conf.Index.External.hasValue()); EXPECT_EQ(Conf.Index.External->Kind, Config::ExternalIndexSpec::File); #endif } +TEST_F(ConfigCompileTests, ExternalBlockDisableWithNone) { + Fragment::IndexBlock::ExternalBlock External; + External.IsNone = true; + Frag.Index.External = std::move(External); + compileAndApply(); + EXPECT_FALSE(Conf.Index.External.hasValue()); +} + TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) { Frag.Index.External.emplace(Fragment::IndexBlock::ExternalBlock{}); compileAndApply(); EXPECT_THAT( Diags.Diagnostics, - Contains(AllOf(DiagMessage("Exactly one of File or Server must be set."), - DiagKind(llvm::SourceMgr::DK_Error)))); + Contains( + AllOf(DiagMessage("Exactly one of File, Server or None must be set."), + DiagKind(llvm::SourceMgr::DK_Error)))); } TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) { diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp index 0c216c208706..d8fbf1a949a9 100644 --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp @@ -147,6 +147,23 @@ horrible ASSERT_THAT(Results, IsEmpty()); } +TEST(ParseYAML, ExternalBlockNone) { + CapturedDiags Diags; + Annotations YAML(R"yaml( +Index: + External: None + )yaml"); + auto Results = + Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback()); + ASSERT_THAT(Diags.Diagnostics, IsEmpty()); + ASSERT_EQ(Results.size(), 1u); + ASSERT_TRUE(Results[0].Index.External); + EXPECT_FALSE(Results[0].Index.External.getValue()->File.hasValue()); + EXPECT_FALSE(Results[0].Index.External.getValue()->MountPoint.hasValue()); + EXPECT_FALSE(Results[0].Index.External.getValue()->Server.hasValue()); + EXPECT_THAT(*Results[0].Index.External.getValue()->IsNone, testing::Eq(true)); +} + TEST(ParseYAML, ExternalBlock) { CapturedDiags Diags; Annotations YAML(R"yaml(