[PATCH] D89670: [clangd] Store the containing symbol for refs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Ref.h:91
   RefKind Kind = RefKind::Unknown;
+  SymbolID Container;
 };
----------------
i am afraid we are going to have an indeterminate value here if for whatever reason `Container` detection fails. (e.g. for macros, or if ASTNode.Parent set to nullptr by libindex).

Sent out https://reviews.llvm.org/D90397 to address the issue.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:779
+  AssertContainer("ref7b", "f5");        // parameter type of function
+}
+
----------------
can we also have an assertion checking containers for top level symbols are the same ?

and extend the test to have multiple declarations inside:
- a namespace
- inside a record (like S1)
and ensure the container is same for those (and different than top-level symbols themselves). It is okay for those symbols.

Note that it is OK for namespaces to be not in the index, i just want to make sure the containers for the top-level symbols inside a namespace are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89670