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