[PATCH] D91860: [clangd] Move remote-index dependency from clangDaemon to ClangdMain
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 20 06:22:34 PST 2020
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
More information about the cfe-commits
mailing list