kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:298
+
+ processRelations(*ND, *ID, Relations);
+
----------------
nridge wrote:
> 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.
yeah just checked the indexing code, that's unfortunate :/.
could you add a comment explaining the situation ?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:431
+ for (const auto &R : Relations) {
+ if (!(R.Roles & static_cast<unsigned>(index::SymbolRole::RelationBaseOf)))
{
+ continue;
----------------
kadircet wrote:
> this might get complicated, maybe isolate it to a `bool
> shouldIndexRelation(const Relation&)`
nit: no need for braces
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+ this->Relations.insert(
+ Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+ }
----------------
nridge wrote:
> 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?
yes I was thinking ahead. just wanted to make sure we can add other relation
types without changing the logic inside `processrelations`.
but it is not that important, let's keep it that way, we can refactor it later
on.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:441
+ auto ObjectID = getSymbolID(Object);
+ if (!ObjectID) {
+ continue;
----------------
nit: braces
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
