kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/index/Ref.h:36 + // one below. + Implicit = 1 << 3, + All = Declaration | Definition | Reference | Implicit, ---------------- instead of doing that, could we rather de-couple two enums completely and have a `symbolRoleToRefKind`(and vice-versa) method instead(we already have some customization in `toRefKind` now) ? as current change increases the risk of overlapping in future(e.g. someone might change symbolrole::declaration and cause failures/regressions in clangd) ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:306 + const auto Spelling = + Lexer::getSpelling(Loc, Buffer, SM, ASTCtx->getLangOpts()); + DeclarationName Name = ND->getDeclName(); ---------------- this might be expensive to trigger while indexing for each reference. could we rather do it as post-processing inside `SymbolCollector::finish` while making use of `TokenBuffer::spelledTokens`(you can create a tokenbuffer through `TokenCollector` with the `PP` inside `SymbolCollector`.) ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:309 + if (Name.isIdentifier() && Name.getAsString() != Spelling) + Roles |= static_cast<uint32_t>(index::SymbolRole::Implicit); + ---------------- It seems like we are only checking for `syntactic` occurence of a symbol. Can we make use of `Spelled` Rather than `Implicit` (which has some semantic meaning) to make that clear? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
