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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
