[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<string> 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<string> 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