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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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