hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/index/Ref.h:32 + // + // class Foo {}; + // ^ Foo declaration ---------------- could you confirm with it? this looks like a `Foo` class definition. ================ Comment at: clang-tools-extra/clangd/index/Ref.h:108 using iterator = const_iterator; + using size_type = size_t; ---------------- nit: I don't understand why we need this change? it seems irrelevant to the patch. ================ 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)) { ---------------- kbobyrev wrote: > hokein wrote: > > 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. > > 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. > > Fair enough, fixed that! > > > CollectRef lambda is a better place to place the FilesToTokensCache logic. > > I don't really agree with that. In my opinion `CollectRef` should remain a > simple helper that simply puts pre-processed reference into the storage. > Complicating it (and also making it work with both `MacroRefs` and `DeclRefs` > while the token spelling check is only required for declarations looks > suboptimal to me. If you really want me to do that, please let me know, but I > personally think it might be better this way. > I don't really agree with that. In my opinion CollectRef should remain a > simple helper that simply puts pre-processed reference into the storage. > Complicating it (and also making it work with both MacroRefs and DeclRefs > while the token spelling check is only required for declarations looks > suboptimal to me. If you really want me to do that, please let me know, but I > personally think it might be better this way. SG. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:191 + Result |= RefKind::Reference; + if (!(Roles & static_cast<unsigned>(index::SymbolRole::Implicit))) + Result |= RefKind::Spelled; ---------------- I was confused at the first time reading the code -- it turns out we reset the flag in the caller. Maybe adjust the function like `RefKind toRefKind(index::SymbolRoleSet Roles, bool isSpelled)`? ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:91 + /// explicitly spell out symbol's name. + bool DropImplicitReferences = false; }; ---------------- what's the use case for this flag? I think we don't need it as we always want all references. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:299 return; + if (!static_cast<unsigned>(R.Kind & RefKind::Spelled)) + return; ---------------- again, I would suggest doing this change in a follow-up patch. ================ Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:711 + $spelled[[Foo]] Variable1; + $macro[[MACRO]] Variable2; + )cpp"); ---------------- could you add a test case for class constructor/destructor to make sure the references are marked `spelled`? ``` class [[Foo]] { [[Foo]](); ~[[Foo]](); } ``` ================ Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:719 + EXPECT_THAT(Refs, Contains(Pair(_, HaveRanges(Main.ranges("macro"))))); + EXPECT_THAT(Refs, SizeIs(2)); +} ---------------- I think we need to verify the `RefKind` in the test, just add a new gtest matcher `IsRefKind`. nit: we can simplify the code by aggregating the above 3 `EXPECT_THAT`s, like ``` EXPECT_THAT(Refs, UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID, HaveRanges(Header.ranges("Foo")))...); ``` ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:324 llvm::ArrayRef<syntax::Token> spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer &Tokens); ---------------- nit: I would make this function come immediately after the above one (without a blank line) to avoid the duplicated documentations, e.g. ``` /// The spelled tokens that overlap or touch a spelling location Loc. /// This always returns 0-2 tokens. llvm::ArrayRef<syntax::Token> spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef<syntax::Token> Tokens); llvm::ArrayRef<syntax::Token> spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer &Tokens); ``` the same below. 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