[PATCH] D62839: [clangd] Index API and implementations for relations

[Kadir Cetinkaya via Phabricator via cfe-commits]

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