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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, mostly good. the only concern from my side is about the `SymbolRole::Implicit`, I think we should get rid of it.



================
Comment at: clang-tools-extra/clangd/index/Ref.h:53
+  // The reference explicitly spells out declaration's name. Such references can
+  // not come from macro expansions or implicit AST nodes.
+  //
----------------
maybe make sense to include an AST example?

```
class Foo { public: Foo(); };

Foo [[f]]; // there is a reference of `Foo` constructor around `f`, this is a non-spelled reference obviously.
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:191
+    Result |= RefKind::Reference;
+  if (!(Roles & static_cast<unsigned>(index::SymbolRole::Implicit)))
+    Result |= RefKind::Spelled;
----------------
kbobyrev wrote:
> 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?
it is not about the amount of code, it is about layering. I think we should not use the `Implicit`.  With the current change, the `SymbolRole::Implicit` comes from two sources:

1) the flag can be set in libindex;
2) the flag can be set in SymbolCollector (via the post-processing in `finish`)

for 1), the implementation of libindex is not completed, only object-c related decls get set. I think our aim is to set the `Spelled` flag if 2) is true.


Another way doing that would be keep the `toRefKind` implementation unchanged, and set the `Spelled` flag afterwards.


================
Comment at: clang-tools-extra/clangd/index/SymbolLocation.h:12
 
+#include "Protocol.h"
 #include "llvm/ADT/StringRef.h"
----------------
I would not do this change, it introduces a new dependency to this header.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:36
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
----------------
do we need this change in this patch? 


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:730
+                          AllOf(HaveRanges(AllRanges),
+                                RefKindIs(RefKind::Spelled, SpelledRanges)))));
+}
----------------
I found the RefKindIs is hard to follow without reading its implementation, I think we can do it in another way, retrieve all spelled refs from `Refs`, and verify them:

```
EXPECT_THAT(SpelledRefs,
             UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                       AllOf(HaveRanges(Main.ranges("spelled"))), SpelledKind()));
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72746/new/

https://reviews.llvm.org/D72746