[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

[Kadir Cetinkaya via Phabricator via cfe-commits]

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