ilya-biryukov added reviewers: sammccall, ilya-biryukov. ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment.
Adding @sammccall, he will definitely want to take a look at index-related changes. On a high level, the approach seems just right. A few questions immedieately that came to mind: - Unconditionally storing much more symbols in the index might have subtle performance implications, especially for our internal use (the codebase is huge). I bet that internally we wouldn't want to store the symbols not needed for completion, so we'll probably need a switch to disable storing them in the indexing implementation. But let's wait for Sam to take a look, he certainly has a better perspective on the issues there. - Current `fuzzyFind` implementation can only match qualifiers strictly (i.e. st::vector won't match std::vector). Should we look into allowing fuzzy matches for this feature? (not in this patch, rather in the long term). - Have you considered also allowing `'.'` and `' '` (space) as separators in the request? Having additional separators doesn't really hurt complexity of the implementation, but allows to switch between tools for different languages easier. E.g., `std.vector.push_back` and `std vector push_back` could produce the same matches as `std::vector::push_back`. ================ Comment at: clangd/ClangdServer.h:163 + StringRef Query, const clangd::WorkspaceSymbolOptions &Opts, + UniqueFunction<void(llvm::Expected<std::vector<SymbolInformation>>)> + Callback); ---------------- NIT: use `Callback` typedef. ================ Comment at: clangd/WorkspaceSymbols.cpp:165 + [](const SymbolInformation &A, const SymbolInformation &B) { + return A.name < B.name; + }); ---------------- We have `FuzzyMatcher`, it can produce decent match scores and is already used in completion. Any reason not to use it here? ================ Comment at: clangd/index/Index.h:141 unsigned References = 0; + // Whether or not the symbol should be considered for completion. ---------------- NIT: remove empty lines to be consistent with the rest of the struct. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits