[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on Windows: http://45.33.8.238/win/28408/step_9.txt

Please take a look, and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc9776c8d4ef7: [clangd] Introduce config compilation for 
External blocks (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D90749?vs=304064=306933#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -9,9 +9,13 @@
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
+#include "Features.inc"
 #include "TestFS.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -20,6 +24,8 @@
 namespace clangd {
 namespace config {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SizeIs;
@@ -196,6 +202,104 @@
 "0");
 }
 
+TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("");
+  External.Server.emplace("");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  llvm::StringLiteral ExpectedDiag =
+#ifdef CLANGD_ENABLE_REMOTE
+  "Exactly one of File or Server must be set.";
+#else
+  "Clangd isn't compiled with remote index support, ignoring Server.";
+#endif
+  EXPECT_THAT(Diags.Diagnostics,
+  Contains(AllOf(DiagMessage(ExpectedDiag),
+ DiagKind(llvm::SourceMgr::DK_Error;
+}
+
+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;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+  Parm.Path = "/foo/bar/baz.h";
+  Frag.Index.Background.emplace("Build");
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("/foo");
+  External.MountPoint.emplace("/foo/bar");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
+  auto GetFrag = [](llvm::StringRef Directory,
+llvm::Optional MountPoint) {
+Fragment Frag;
+Frag.Source.Directory = Directory.str();
+Fragment::IndexBlock::ExternalBlock External;
+External.File.emplace("/foo");
+if (MountPoint)
+  External.MountPoint.emplace(*MountPoint);
+Frag.Index.External = std::move(External);
+return Frag;
+  };
+
+  Parm.Path = "/foo/bar.h";
+  // Non-absolute MountPoint without a directory raises an error.
+  Frag = GetFrag("", "foo");
+  compileAndApply();
+  ASSERT_THAT(
+  Diags.Diagnostics,
+  ElementsAre(
+  AllOf(DiagMessage("MountPoint must be an absolute path, because this "
+"fragment is not associated with any directory."),
+DiagKind(llvm::SourceMgr::DK_Error;
+  ASSERT_FALSE(Conf.Index.External);
+
+  // Ok when relative.
+  Frag = GetFrag("/", "foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+
+  // None defaults to ".".
+  Frag = GetFrag("/", llvm::None);
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/");
+
+  // Without a file, external index is empty.
+  Parm.Path = "";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File outside MountPoint, no index.
+  Parm.Path = "/bar/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File under MountPoint, index should be set.
+  Parm.Path = "/foo/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+}
 } // namespace
 } // namespace config
 } // namespace clangd
Index: 

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 304064.
kadircet marked 7 inline comments as done.
kadircet added a comment.

- Use ExternalBlock's range for diags.
- Move diagnostic emitting to makeAbsolute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -9,9 +9,13 @@
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
+#include "Features.inc"
 #include "TestFS.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -20,6 +24,8 @@
 namespace clangd {
 namespace config {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SizeIs;
@@ -176,6 +182,105 @@
 ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   }
 }
+
+TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("");
+  External.Server.emplace("");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  llvm::StringLiteral ExpectedDiag =
+#ifdef CLANGD_ENABLE_REMOTE
+  "Exactly one of File or Server must be set.";
+#else
+  "Clangd isn't compiled with remote index support, ignoring Server.";
+#endif
+  EXPECT_THAT(Diags.Diagnostics,
+  Contains(AllOf(DiagMessage(ExpectedDiag),
+ DiagKind(llvm::SourceMgr::DK_Error;
+}
+
+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;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+  Parm.Path = "/foo/bar/baz.h";
+  Frag.Index.Background.emplace("Build");
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("/foo");
+  External.MountPoint.emplace("/foo/bar");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
+  auto GetFrag = [](llvm::StringRef Directory,
+llvm::Optional MountPoint) {
+Fragment Frag;
+Frag.Source.Directory = Directory.str();
+Fragment::IndexBlock::ExternalBlock External;
+External.File.emplace("/foo");
+if (MountPoint)
+  External.MountPoint.emplace(*MountPoint);
+Frag.Index.External = std::move(External);
+return Frag;
+  };
+
+  Parm.Path = "/foo/bar.h";
+  // Non-absolute MountPoint without a directory raises an error.
+  Frag = GetFrag("", "foo");
+  compileAndApply();
+  ASSERT_THAT(
+  Diags.Diagnostics,
+  ElementsAre(
+  AllOf(DiagMessage("MountPoint must be an absolute path, because this "
+"fragment is not associated with any directory."),
+DiagKind(llvm::SourceMgr::DK_Error;
+  ASSERT_FALSE(Conf.Index.External);
+
+  // Ok when relative.
+  Frag = GetFrag("/", "foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+
+  // None defaults to ".".
+  Frag = GetFrag("/", llvm::None);
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/");
+
+  // Without a file, external index is empty.
+  Parm.Path = "";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File outside MountPoint, no index.
+  Parm.Path = "/bar/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File under MountPoint, index should be set.
+  Parm.Path = "/foo/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+}
 } // namespace
 } // namespace config
 } // namespace clangd
Index: clang-tools-extra/clangd/ConfigCompile.cpp

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:252
+
+  llvm::Expected makeAbsolute(std::string Path,
+   llvm::StringRef Base,

consistent with other helpers, can we avoid passing errors around, and instead 
issue the diagnostic here and return optional?

Signature seems overly general: the Path argument is always a Located, 
the Base is always the fragment directory. These seem likely to always hold.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:265
+llvm::SmallString<256> AbsPath;
+llvm::sys::path::append(AbsPath, FragmentDirectory, Path);
+llvm::sys::path::native(AbsPath, Style);

why not path::make_absolute?



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:292
+  Spec.Kind = Config::ExternalIndexSpec::File;
+  // Make sure File is an absolute native path.
+  auto AbsPath = makeAbsolute(std::move(**External.File), 
FragmentDirectory,

comment echoes the code, which is not hard to read - remove?



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:256
+  diag(Error, "At least one of File or Server must be specified.",
+   llvm::SMRange());
+  return;

kadircet wrote:
> sammccall wrote:
> > these missing ranges are potentially pretty difficult for users - they 
> > won't know e.g. which file the error occurred in.
> > 
> > Do you think we can `Located<>` the whole `ExternalBlock`?
> I was doing that initially, but they felt similarly annoying, as they point 
> at value corresponding to `External` block rather than the whole {key,value} 
> range. In addition to that it only covers the first line :/
> 
> Happy to do that if you think that's still better than an empty/invalid range.
I think it's better than the empty range, even if it just gives us a location 
for the block, that tells us which file and which fragment.

(sorry, not sure what "`,value` range" is...)



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:305
+
+  // Disable background indexing for the files under the mountpoint.
+  // Note that this will overwrite statements in any previous fragments

kadircet wrote:
> sammccall wrote:
> > hang on, you say "under the mountpoint", but... that's not what's happening.
> > 
> > If we want that behavior, we need to use SourceInfo::Directory as the 
> > mountpoint (and so probably give users a way to set that in the global 
> > config fragment).
> > hang on, you say "under the mountpoint", but... that's not what's happening.
> 
> Umm, why? We bail-out at the beginning of apply if filepath doesn't start 
> with mountpoint ?
Sorry, I was confused...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 303956.
kadircet added a comment.

- Define an ExternalIndexSpec structure, and populate that during compile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -9,9 +9,13 @@
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
+#include "Features.inc"
 #include "TestFS.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -20,6 +24,8 @@
 namespace clangd {
 namespace config {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SizeIs;
@@ -176,6 +182,105 @@
 ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   }
 }
+
+TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("");
+  External.Server.emplace("");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  llvm::StringLiteral ExpectedDiag =
+#ifdef CLANGD_ENABLE_REMOTE
+  "Exactly one of File or Server must be set.";
+#else
+  "Clangd isn't compiled with remote index support, ignoring Server.";
+#endif
+  EXPECT_THAT(Diags.Diagnostics,
+  Contains(AllOf(DiagMessage(ExpectedDiag),
+ DiagKind(llvm::SourceMgr::DK_Error;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) {
+  Frag.Index.External.emplace();
+  compileAndApply();
+  EXPECT_THAT(
+  Diags.Diagnostics,
+  Contains(AllOf(DiagMessage("Exactly one of File or Server must be set."),
+ DiagKind(llvm::SourceMgr::DK_Error;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+  Parm.Path = "/foo/bar/baz.h";
+  Frag.Index.Background.emplace("Build");
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("/foo");
+  External.MountPoint.emplace("/foo/bar");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
+  auto GetFrag = [](llvm::StringRef Directory,
+llvm::Optional MountPoint) {
+Fragment Frag;
+Frag.Source.Directory = Directory.str();
+Fragment::IndexBlock::ExternalBlock External;
+External.File.emplace("/foo");
+if (MountPoint)
+  External.MountPoint.emplace(*MountPoint);
+Frag.Index.External = std::move(External);
+return Frag;
+  };
+
+  Parm.Path = "/foo/bar.h";
+  // Non-absolute MountPoint without a directory raises an error.
+  Frag = GetFrag("", "foo");
+  compileAndApply();
+  ASSERT_THAT(
+  Diags.Diagnostics,
+  ElementsAre(
+  AllOf(DiagMessage("MountPoint must be an absolute path, because this "
+"fragment is not associated with any directory."),
+DiagKind(llvm::SourceMgr::DK_Error;
+  ASSERT_FALSE(Conf.Index.External);
+
+  // Ok when relative.
+  Frag = GetFrag("/", "foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+
+  // None defaults to ".".
+  Frag = GetFrag("/", llvm::None);
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/");
+
+  // Without a file, external index is empty.
+  Parm.Path = "";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File outside MountPoint, no index.
+  Parm.Path = "/bar/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File under MountPoint, index should be set.
+  Parm.Path = "/foo/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+}
 } // namespace
 } // namespace config
 } // namespace clangd
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ 

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 5 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:256
+  diag(Error, "At least one of File or Server must be specified.",
+   llvm::SMRange());
+  return;

sammccall wrote:
> these missing ranges are potentially pretty difficult for users - they won't 
> know e.g. which file the error occurred in.
> 
> Do you think we can `Located<>` the whole `ExternalBlock`?
I was doing that initially, but they felt similarly annoying, as they point at 
value corresponding to `External` block rather than the whole {key,value} 
range. In addition to that it only covers the first line :/

Happy to do that if you think that's still better than an empty/invalid range.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:305
+
+  // Disable background indexing for the files under the mountpoint.
+  // Note that this will overwrite statements in any previous fragments

sammccall wrote:
> hang on, you say "under the mountpoint", but... that's not what's happening.
> 
> If we want that behavior, we need to use SourceInfo::Directory as the 
> mountpoint (and so probably give users a way to set that in the global config 
> fragment).
> hang on, you say "under the mountpoint", but... that's not what's happening.

Umm, why? We bail-out at the beginning of apply if filepath doesn't start with 
mountpoint ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:254
+// native path.
+if (!External.File && !External.Server) {
+  diag(Error, "At least one of File or Server must be specified.",

My math friends would call this "case bashing" :-)
In particular I'm cringing at the thought of adding a third option here (it's 
not likely, but I think the code would be easier to understand if it was clear 
that two is an arbitrary number).

What about:
```
unsigned SourceCount = External.File.hasValue() + External.Server.hasValue();
if (SourceCount != 1)
  return diag(Error, "Exactly one of File and Server must be set");
```
(I don't think separate error messages for missing/multiple, or any particular 
behavior for multiple, is particularly useful)



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:256
+  diag(Error, "At least one of File or Server must be specified.",
+   llvm::SMRange());
+  return;

these missing ranges are potentially pretty difficult for users - they won't 
know e.g. which file the error occurred in.

Do you think we can `Located<>` the whole `ExternalBlock`?



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:270
+  AbsPath.clear();
+  llvm::sys::path::append(AbsPath, FragmentDirectory, **External.File);
+  External.File.emplace(AbsPath.str().str());

somewhere we should be converting the paths to native slashes, I think.



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:283
+  if (FragmentDirectory.empty()) {
+diag(Error, "MountPoint must be an absolute path.",
+ External.MountPoint->Range);

pull out a method like `Optional makeAbsolute(StringRef Path, StringRef 
Description)`?



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:283
+  if (FragmentDirectory.empty()) {
+diag(Error, "MountPoint must be an absolute path.",
+ External.MountPoint->Range);

sammccall wrote:
> pull out a method like `Optional makeAbsolute(StringRef Path, 
> StringRef Description)`?
would also be good to include "Because this configuration is not associated 
with a directory"



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:300
+  C.Index.External.MountPoint = std::move(**External.MountPoint);
+  if (External.File)
+C.Index.External.File.emplace(std::move(**External.File));

we do all this validation and wind up writing back into the same data structure 
that can't represent it.
We don't actually have to validate again later, because now our invariants are 
rules, not suggestions. Still, it seems fragile, and violates a maxim that got 
stuck in my head: 
https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/

Can we have an type for this spec in config, and have only a single slot for it?
It could be as simple as:
```
struct ExternalIndexSpec { 
  enum {File, Remote} Kind;
  std::string Location; // specific to kind, hack this up later if need be
  std::string MountPoint;
};
```

I think the single biggest improvement is that we'd model "replacing the index 
for this config" as a single assignment rather than 5 coordinated ones. So if 
you want something less different, putting the `Optional` around the config 
struct instead of individual fields would be fine too. (though that would 
require it to be named)



Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:305
+
+  // Disable background indexing for the files under the mountpoint.
+  // Note that this will overwrite statements in any previous fragments

hang on, you say "under the mountpoint", but... that's not what's happening.

If we want that behavior, we need to use SourceInfo::Directory as the 
mountpoint (and so probably give users a way to set that in the global config 
fragment).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90749/new/

https://reviews.llvm.org/D90749

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kadircet requested review of this revision.
Herald added subscribers: ormris, MaskRay, ilya-biryukov.

Compilation logic for External blocks. A few of the high level points:

- Requires exactly one-of File/Server at a time:
  - Server is ignored in case of both, with a warning.
  - Having none is an error, would render ExternalBlock void.
- Ensures mountpoint is an absolute path:
  - Interprets it as relative to FragmentDirectory.
  - Defaults to FragmentDirectory when empty.
- Marks Background as Skip.

Depends on D90748 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90749

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -10,8 +10,11 @@
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
 #include "TestFS.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -20,6 +23,8 @@
 namespace clangd {
 namespace config {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SizeIs;
@@ -176,6 +181,97 @@
 ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   }
 }
+
+TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("");
+  External.Server.emplace("");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_THAT(
+  Diags.Diagnostics,
+  Contains(
+  AllOf(DiagMessage("Both File and Server specified, ignoring Server."),
+DiagKind(llvm::SourceMgr::DK_Warning;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) {
+  Frag.Index.External.emplace();
+  compileAndApply();
+  EXPECT_THAT(
+  Diags.Diagnostics,
+  Contains(AllOf(
+  DiagMessage("At least one of File or Server must be specified."),
+  DiagKind(llvm::SourceMgr::DK_Error;
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+  Parm.Path = "/foo/bar/baz.h";
+  Frag.Index.Background.emplace("Build");
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("/foo");
+  External.MountPoint.emplace("/foo/bar");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
+  auto GetFrag = [](llvm::StringRef Directory,
+llvm::Optional MountPoint) {
+Fragment Frag;
+Frag.Source.Directory = Directory.str();
+Fragment::IndexBlock::ExternalBlock External;
+External.File.emplace("/foo");
+if (MountPoint)
+  External.MountPoint.emplace(*MountPoint);
+Frag.Index.External = std::move(External);
+return Frag;
+  };
+
+  Parm.Path = "/foo/bar.h";
+  // Non-absolute MountPoint without a directory raises an error.
+  Frag = GetFrag("", "foo");
+  compileAndApply();
+  ASSERT_THAT(
+  Diags.Diagnostics,
+  ElementsAre(AllOf(DiagMessage("MountPoint must be an absolute path."),
+DiagKind(llvm::SourceMgr::DK_Error;
+  EXPECT_THAT(Conf.Index.External.MountPoint, IsEmpty());
+
+  // Ok when relative.
+  Frag = GetFrag("/", "foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.Index.External.MountPoint, "/foo");
+
+  // None defaults to ".".
+  Frag = GetFrag("/", llvm::None);
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.Index.External.MountPoint, "/");
+
+  // Without a file, external index is empty.
+  Parm.Path = "";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.Index.External.MountPoint, IsEmpty());
+
+  // File outside MountPoint, no index.
+  Parm.Path = "/bar/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.Index.External.MountPoint, IsEmpty());
+
+  // File under MountPoint, index should be set.
+  Parm.Path = "/foo/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.Index.External.MountPoint, "/foo");
+}
 } // namespace
 } // namespace config
 } // namespace clangd
Index: