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

[Haojian Wu via Phabricator via cfe-commits]

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