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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
