[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 7 inline comments as done.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair<RelationKey, llvm::ArrayRef<SymbolID>>;
+  using const_iterator = std::vector<value_type>::const_iterator;
----------------
kadircet wrote:
> gribozavr wrote:
> > `struct Relation`?  And in the comments for it, please explain which way the relationship is directed (is the SymbolID in the key the subtype?  or is the SymbolID in the ArrayRef the subtype?).
> Ah exactly my thoughts, forget to mention this.
> 
> I believe current usage is the counter-intuitive one. For example, we will most likely query with something like:
> `getRelations(SymbolID, baseOf)` to get all relations where `SymbolID` is `baseOf` something else(which says get children of `SymbolID`)
> 
> So that this valueType can be read like, 
> ```
> `SymbolID` is `RelationKind` every `SymbolID inside array`
> ```
> WDYT?
The way I was thinking of it is that `getRelations(SymbolID, baseOf)` would return all the bases of `SymbolID`. However, the opposite interpretation is also reasonable, we just need to pick one and document it. I'm happy to go with your suggested one.


================
Comment at: clang-tools-extra/clangd/index/Index.h:59
+    return sizeof(*this) + Arena.getTotalMemory() +
+           sizeof(value_type) * Relations.size();
+  }
----------------
kadircet wrote:
> use capacity instead of size
Note, `RefSlab::bytes()` (which I where I copied this from) uses `size()` as well. Should I change that too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407