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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> (Meta-point: not sure how useful splitting this patch out from the compile step is...)

I wanted to keep them separate especially to ensure testing isn't mixed up. As these things tend to get big easily :/



================
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`.
----------------
sammccall wrote:
> Should we mention slashes here too? :-\
> 
> Maybe we should just lift "all paths use forward-slashes" to a top-level comment.
This is literally used for pointing at a file on disk though. So it doesn't really matter if this is forward or back slashes?


================
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.
----------------
sammccall wrote:
> 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"...)
not sure where else we repeated this ?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90748