hokein added a comment. In D72746#1821097 <https://reviews.llvm.org/D72746#1821097>, @kbobyrev wrote:
> The patch could be shorter and slightly less confusing if I preserved 1:1 > `RefKind::Implicit` <-> `index::SymbolKind::Implicit` relation, but I thought > we might want to keep `RefKind` being 1 byte so that we don't waste > unnecessary memory. I think it is time to create our own `Declaration`, `Definition` in our `RefKind` (rather than reusing the enums in libindex). > Also, since we might want to start using `findExplicitReferences` (or > something else we come up with) for indexing, it shouldn't be critical. Let > me know if you think it's unnecessary. Yeah, we may have a plan in the future, but I don't think switching to `findExplicitReferences` is trivial, there are a few things we need to figure out carefully, e.g. how to support macros, implicit references. ================ Comment at: clang-tools-extra/clangd/index/Ref.h:33 Reference = static_cast<uint8_t>(index::SymbolRole::Reference), - All = Declaration | Definition | Reference, + // This corresponds to index::SymbolRole::Implicit. In order to save memory + // and keep RefKind occupying 1 byte, the original value is modified to the ---------------- I'm skeptical about definition of `index::SymbolRole::Implicit`. I think the `implicit` we want here is references that are spelled/written as the same as the named decls, so it is more likely to align with `index::SymbolRole::NameReferences`. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:309 + if (Name.isIdentifier() && Name.getAsString() != Spelling) + Roles |= static_cast<uint32_t>(index::SymbolRole::Implicit); + ---------------- kadircet wrote: > 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? +1, `Implicit` is ambiguous, maybe `Named`, `Explicit` (this reflects to the `findExplicitReferences`)? 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
