[PATCH] D90748: [clangd] Introduce config parsing for External blocks

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LG (comment nits) thanks!
(Meta-point: not sure how useful splitting this patch out from the compile step is...)



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:165
     llvm::Optional<Located<std::string>> Background;
+    /// Configuration information for an external index. If both File and Server
+    /// are set, Server will be ignored. This will be used by index
----------------
I'd move the file-take-precedence-over-server part to server.

Or even just say only one source (file/server) should be configured, leave the behavior unspecified, and report an error when compiling the fragment.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:165
     llvm::Optional<Located<std::string>> Background;
+    /// Configuration information for an external index. If both File and Server
+    /// are set, Server will be ignored. This will be used by index
----------------
sammccall wrote:
> I'd move the file-take-precedence-over-server part to server.
> 
> Or even just say only one source (file/server) should be configured, leave the behavior unspecified, and report an error when compiling the fragment.
"Configuration information for" is redundant here.

maybe just

/// An external index uses data source outside of clangd itself.
/// This is usually prepared using clangd-indexer.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:170
+    struct ExternalBlock {
+      /// Path to a monolithic index on disk. Absolute in case of a global
+      /// config, relative to location the config file containing the fragment
----------------
nit: I'm not sure "monolithic" is a useful qualifier to the audience here.
Path to an index file generated by clangd-indexer?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:171
+      /// Path to a monolithic index on disk. Absolute in case of a global
+      /// config, relative to location the config file containing the fragment
+      /// otherwise.
----------------
Generalize this as "Relative paths may be used, if the config fragment is associated with a directory."?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:173
+      /// otherwise.
+      llvm::Optional<Located<std::string>> File;
+      /// Address and port number for a remote-index. e.g. `123.1.1.1:13337`.
----------------
Should we mention slashes here too? :-\

Maybe we should just lift "all paths use forward-slashes" to a top-level comment.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:174
+      llvm::Optional<Located<std::string>> File;
+      /// Address and port number for a remote-index. e.g. `123.1.1.1:13337`.
+      llvm::Optional<Located<std::string>> Server;
----------------
nit: for a clangd-index-server


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:176
+      llvm::Optional<Located<std::string>> Server;
+      /// Source root governed by this index. None implies current config file
+      /// location. Absolute in case of user config and relative otherwise.
----------------
Again: "Default is the directory associated with the config frament".

(Repeating this makes me wonder if we should have defined a more specific name like "Home"...)


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:103
+#else
+      warning("Remote index support isn't enabled for this clangd.", N);
+#endif
----------------
nit: this sort of validation would normally live in ConfigCompile, since it's not to do with serialization


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90748