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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
