[PATCH] D59407: [clangd] Add RelationSlab

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 03:51:34 PDT 2019


kadircet added a comment.

In D59407#1432543 <https://reviews.llvm.org/D59407#1432543>, @nridge wrote:

> If it's (B), how many bytes should the index be? Are the space gains worth the complexity, given that `SymbolID` is only 8 bytes to begin with? (As compared to say, the filenames in `Ref`, which can be much longer, making this sort of optimization more clearly worth it.)


That was the case, I agree with you we should rather do that while serializing where we have variable length integers and duplication is more severe(all three slabs make use of same SymbolID pool).



================
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;
----------------
nridge wrote:
> 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.
It looks like IndexingAPI is also using the interpretation I suggested, so let's move with that one if you don't have any other concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407





More information about the cfe-commits mailing list