[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