[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

[Kirill Bobyrev via Phabricator via cfe-commits]

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