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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Index.h:107
+  virtual void
+  relations(const SymbolID &Subject, index::SymbolRole Predicate,
+            llvm::function_ref<void(const SymbolID &)> Callback) const = 0;
----------------
kadircet wrote:
> We might need additional options, like limiting number of results. Could you instead accept a `RelsRequest` object? You can check `RefsRequest` for a sample
I called it `RelationsRequest` to avoid visual confusion with `RefsRequest`.


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:29
       this->Refs.try_emplace(R.first, R.second.begin(), R.second.end());
+    RelationSlab::Builder Builder;
+    for (const Relation &R : Relations)
----------------
kadircet wrote:
> why not just store `densemap<<s,p>,arrayref<o>>` ?
I used `std::vector<o>` because it wasn't clear where the array would live otherwise.


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:126
+    llvm::function_ref<void(const SymbolID &)> Callback) const {
+  // Return results from both indexes but avoid duplicates.
+  llvm::DenseSet<SymbolID> SeenObjects;
----------------
kadircet wrote:
> avoiding deduplicates is not enough, we also wouldn't want to report stale relations coming from static index.
> 
> Could you check the logic in `MergedIndex::refs`, and hopefully refactor it into a class to share between these two?
The description in `MergedIndex::refs()` says:

```
  // We don't want duplicated refs from the static/dynamic indexes,
  // and we can't reliably deduplicate them because offsets may differ slightly.
  // We consider the dynamic index authoritative and report all its refs,
  // and only report static index refs from other files.
```

It seems to me that the problem of "can't reliably deduplicate them because offsets may differ slightly" doesn't apply to relations, as there are no offsets involved. So, is there still a need to have additional logic here?

If so, what would the logic be -- don't return an object O1 from the static index if we've returned an object O2 from the dynamic index defined in the same file as O1? (Note that to implement this, we'd have to additionally look up each SymbolID we return in the symbol table, as we don't otherwise know what file the object is located in.)


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:28
+                                        RelationSlab Rels) {
   auto Size = Symbols.bytes() + Refs.bytes();
+  // There is no need to include "Rels" in Data because the relations are self-
----------------
kadircet wrote:
> `+rels.size()`
`Size` is only the backing data size, so if we don't include the relations in the backing data, we shouldn't include `Rels.size()`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62839