nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:298
+
+ processRelations(*ND, *ID, Relations);
+
----------------
kadircet wrote:
> why do we want to process these relations for references?
The `RelationBaseOf` is not present in the `Relations` parameter of the
`handleDeclOccurrence()` call for the base class's declaration. It's only
present for the specific reference that's in the base-specifier.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:436
+
+ if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Parent)) {
+ if (!CTSD->isExplicitSpecialization()) {
----------------
kadircet wrote:
> why? I believe we have those in the index since rL356125.
>
> even if we don't I believe this part should also be isolated to somehow get
> canonical version of a given symbol.
I removed this logic as it no longer seems to be necessary now that we use
libIndex's relations support to compute our relations.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+ this->Relations.insert(
+ Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+ }
----------------
kadircet wrote:
> kadircet wrote:
> > I believe subject is `ParentID` and object is `ID` in a baseof relation so
> > these should be replaced ?
> also we should generalize this with something like
> `index::applyForEachSymbolRole`, which should also get rid of the check at
> top(`shouldIndexRelation`)
> I believe subject is ParentID and object is ID in a baseof relation so these
> should be replaced ?
The code was functionally correct, but the name `ParentID` was wrong -- the
`RelatedSymbol` for a `BaseOf` relation is the child.
Anyways, I'm now calling it `ObjectID`.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+ this->Relations.insert(
+ Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+ }
----------------
nridge wrote:
> kadircet wrote:
> > kadircet wrote:
> > > I believe subject is `ParentID` and object is `ID` in a baseof relation
> > > so these should be replaced ?
> > also we should generalize this with something like
> > `index::applyForEachSymbolRole`, which should also get rid of the check at
> > top(`shouldIndexRelation`)
> > I believe subject is ParentID and object is ID in a baseof relation so
> > these should be replaced ?
>
> The code was functionally correct, but the name `ParentID` was wrong -- the
> `RelatedSymbol` for a `BaseOf` relation is the child.
>
> Anyways, I'm now calling it `ObjectID`.
> also we should generalize this with something like
> index::applyForEachSymbolRole, which should also get rid of the check at
> top(shouldIndexRelation)
I don't fully understand this suggestion; could you elaborate?
Are you thinking ahead to a future where we want to index additional kinds of
relations? Can we refactor things at that time?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62471/new/
https://reviews.llvm.org/D62471
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
