[PATCH] D59407: [clangd] Add RelationSlab

Nathan Ridge via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 21 19:01:58 PDT 2019


nridge marked 8 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:
> 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.
Already updated to this interpretation :)


================
Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+    RelationKey Key;
----------------
gribozavr wrote:
> Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)
This comment made me realize that I haven't addressed your previous comment properly: I haven't changed `RelationSlab::value_type` from `std::pair<RelationKey, llvm::ArrayRef<SymbolID>>` to `Relation`.

I tried to make that change this time, and ran into a problem:

In the rest of the subtypes patch (D58880), one of the things I do is extend the `MemIndex` constructor so that, in addition to taking a symbol range and a ref range, it takes a relation range.

That constructor assumes that the elements of that range have members of some name - either `first` and `second` (as currently in D58880), or `Key` and `Value`.

However, that constructor has two call sites. In `MemIndex::build()`, we pass in the slabs themselves as the ranges. So, if we make this change, the field names for that call site will be `Key` and `Value`. However, for the second call site in `FileSymbols::buildIndex()`, the ranges that are passed in are `DenseMap`s, and therefore their elements' field names are necessarily `first` and `second`. The same constructor cannot therefore accept both ranges.

How do you propose we address this?

 * Scrap `struct Relation`, and keep `value_type` as `std::pair<RelationKey, llvm::ArrayRef<SymbolID>>`?
 * Keep `struct Relation`, but make its fields named `first` and `second`?
 * Split the constructor of `MemIndex` into two constructors, to accomodate both sets of field names?
 * Something else?


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