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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
