kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/Ref.h:32
+ //
+ // class Foo {};
+ // ^ Foo declaration
----------------
hokein wrote:
> could you confirm with it? this looks like a `Foo` class definition.
Good point, I thought it should be both.
================
Comment at: clang-tools-extra/clangd/index/Ref.h:108
using iterator = const_iterator;
+ using size_type = size_t;
----------------
hokein wrote:
> nit: I don't understand why we need this change? it seems irrelevant to the
> patch.
This one is required for `SizeIs` matcher, but I believe I could use
`UnorderedAre` instead (to make sure there are no extra references).
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:191
+ Result |= RefKind::Reference;
+ if (!(Roles & static_cast<unsigned>(index::SymbolRole::Implicit)))
+ Result |= RefKind::Spelled;
----------------
hokein wrote:
> 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)`?
If my understanding is correct, the suggestion is to take `isSpelled` argument
and apply the `RefKind` flag based on the value of the argument instead of
using `SymbolRole::Implicit`. Is that right? In this case I would be concerned
about doing because that increase the amount of code in case there is any other
index provider that sets the flags itself.
What do you think?
================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:324
llvm::ArrayRef<syntax::Token>
spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer &Tokens);
----------------
hokein wrote:
> 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.
Makes sense. I was concerned about Doxygen not generating comments for both of
these functions, but I think it should be OK.
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
