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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the scope of this patch is not very clear, the changes touch two different code parts `SymbolCollector`, and `Rename`, and we are lacking tests for `SymbolCollector`. I'd suggest spliting this patch into smaller patches:

- a patch that adds a new kind to the ref, and updates the `SymbolCollector`
- a patch that updates the rename accordingly



================
Comment at: clang-tools-extra/clangd/index/Ref.h:33
+  Reference = 1 << 2,
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
----------------
could you please add some brief documentation on these fields?


================
Comment at: clang-tools-extra/clangd/index/Ref.h:34
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
 };
----------------
The `All` now indicates all spelled refs. I think `All` should include both non-spelled and spell refs, which should be `declaration | Definition | Reference`.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:591
+  const auto MainFileID = SM.getMainFileID();
+  if (const auto MainFileURI = GetURI(MainFileID)) {
+    assert(ASTCtx && "ASTContext must be set.");
----------------
looks like we don't use the `MainFileURI` variable below, I think we can remove this `if` statement.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:598
+          syntax::tokenize(MainFileID, SM, ASTCtx->getLangOpts());
+    const auto Tokens = FilesToTokensCache[MainFileID];
+    for (auto &DeclAndRef : DeclRefs) {
----------------
since we only use `FilesToTokensCache` in this function, make it as a local variable rather than class member.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:599
+    const auto Tokens = FilesToTokensCache[MainFileID];
+    for (auto &DeclAndRef : DeclRefs) {
+      if (auto ID = getSymbolID(DeclAndRef.first)) {
----------------
note that the `DeclRefs` may contains references from non-main files, e.g. included headers if `RefsInHeaders` is true. I think we need to tokenize other files if the reference is not from main file.   `CollectRef` lambda is a better place to place the `FilesToTokensCache` logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746