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