[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<std::string> 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<string>, 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