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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Can you also add some tests ?

we have some tests in `unittests/SymbolCollectorTests.cpp`



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:298
+
+  processRelations(*ND, *ID, Relations);
+
----------------
why do we want to process these relations for references?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:426
+  // Store subtype relations.
+  const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(&ND);
+  if (!RD)
----------------
maybe rather check `TagDecl` to be as generic as possible.

also the check can be inlined since you are not using RD afterwards, i.e 
```
if(!dyn_cast<TagDecl>(&ND))
  return;
```


================
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;
----------------
this might get complicated, maybe isolate it to a `bool shouldIndexRelation(const Relation&)`


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:434
+    }
+    const Decl *Parent = R.RelatedSymbol;
+
----------------
maybe call it `Object` ?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:436
+
+    if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Parent)) {
+      if (!CTSD->isExplicitSpecialization()) {
----------------
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.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:441
+    }
+    if (auto ParentID = getSymbolID(Parent)) {
+      // We are a subtype to each of our parents.
----------------
nit: invert the condition to reduce nesting


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+      this->Relations.insert(
+          Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+    }
----------------
I believe subject is `ParentID` and object is `ID` in a baseof relation so these should be replaced ?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:453
+      this->Relations.insert(
+          Relation{ID, index::SymbolRole::RelationBaseOf, *ParentID});
+    }
----------------
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`)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62471/new/

https://reviews.llvm.org/D62471