[PATCH] D91860: [clangd] Move remote-index dependency from clangDaemon to ClangdMain

[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/index/ProjectAware.h:27
 /// Returns an index that answers queries using external indices. IndexGenerator
 /// can be used to customize how to generate an index from an external source.
+std::unique_ptr<SymbolIndex> createProjectAwareIndex(IndexFactory);
----------------
while here, IndexGenerator -> IndexFactory, and "can be use to customize" --> "specifies" now that it's mandatory


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:558
 
+// We define this function here as depending on clangdRemoteIndex within
+// clangDaemon introduces a cyclic dependency.
----------------
This sort of a comment seems to be rebutting the previous version of the code, which is important today but probably not in the long run.

It may not be needed at all - this factoring actually looks more natural than I expected!
If it is, I think it belongs on createProjectAwareIndex rather than here, as an aside.
`(IndexFactory must be injected because this code cannot depend on the remote index client)`


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:561
+std::unique_ptr<SymbolIndex>
+projectAwareIndexFactory(const Config::ExternalIndexSpec &External,
+                         AsyncTaskRunner &Tasks) {
----------------
maybe just loadExternalIndex, which more clearly describes what it *does* as opposed to where it's passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91860