[PATCH] D62471: [clangd] SymbolCollector support for relations

[Nathan Ridge via Phabricator via cfe-commits]

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