kadircet added a comment.
LGTM, except the batch query support.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97
+
+SymbolSlab indexHeaderSymbols(ASTContext &AST, std::shared_ptr<Preprocessor>
PP,
+ const CanonicalIncludes &Includes) {
----------------
we should have only a couple call sites to this function. Let's rather change
those instead of introducing a new helper.
================
Comment at: clang-tools-extra/clangd/index/Index.h:77
+struct RelationsRequest {
+ SymbolID Subject;
+ index::SymbolRole Predicate;
----------------
sorry for missing it in previous iteration. but this should also enable batch
queries. let's make this one a `llvm::DenseMap<SymbolID>`.
And in the callback we should output both the `SymbolID` of the `Subject` and
`Object`.
We have a network based index(internally) and it uses this batch query
optimization to get rid of network latency.
================
Comment at: clang-tools-extra/clangd/index/Index.h:113
+ /// Finds all symbol IDs 'Object' such that (Req.Subject, Req.Predicate,
+ /// Object) forms a relation stored in the index, and applies \p Callback
----------------
I think the old comment is better. No need to expose internals.
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:244
llvm::function_ref<void(const Symbol &)> Callback) const {
trace::Span Tracer("Dex lookup");
for (const auto &ID : Req.IDs) {
----------------
could you revert these changes?
================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:268
+ llvm::function_ref<void(const Symbol &)> Callback) const {
+ LookupRequest LookupReq;
+ uint32_t Remaining =
----------------
can you also add a span
```
trace::Span Tracer("Dex relations");
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62839/new/
https://reviews.llvm.org/D62839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
