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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 242809.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Address another round of comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -316,11 +316,16 @@
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+const syntax::Token *
 spelledIdentifierTouching(SourceLocation Loc,
   const syntax::TokenBuffer );
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -700,6 +700,41 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  struct $spelled[[Foo]] {
+$spelled[[Foo]]();
+~$spelled[[Foo]]();
+  };
+  $spelled[[Foo]] Variable1;
+  $implicit[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = false;
+  runSymbolCollector(Header.code(), Main.code());
+  const auto SpelledRanges = Main.ranges("spelled");
+  const auto ImplicitRanges = Main.ranges("implicit");
+  RefSlab::Builder SpelledSlabBuilder, ImplicitSlabBuilder;
+  for (const auto  : Refs) {
+const auto Symbol = SymbolAndRefs.first;
+for (const auto  : SymbolAndRefs.second)
+  if ((Ref.Kind & RefKind::Spelled) != RefKind::Unknown)
+SpelledSlabBuilder.insert(Symbol, Ref);
+  else
+ImplicitSlabBuilder.insert(Symbol, Ref);
+  }
+  const auto SpelledRefs = std::move(SpelledSlabBuilder).build(),
+ ImplicitRefs = std::move(ImplicitSlabBuilder).build();
+  EXPECT_THAT(SpelledRefs, 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

thanks, looks good, a few nits.

before landing the patch, could you please run the clangd-indexer on the llvm 
project and measure the before/after indexing time? to make sure we don't 
introduce a big latency.
you can run the command like  `time ./bin/clangd-indexer -format=binary 
-executor=all-TUs . > static-index.idx`




Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:588
 for (const auto  : IDAndRefs.second)
   CollectRef(IDAndRefs.first, LocAndRole);
   // Populate Refs slab from DeclRefs.

macro is tricky, macro references are not marked `spelled`, it is OK for now as 
we are not interested in them, but I think most of time they are spelled in the 
source code. Maybe add a comment?



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:603
+// corresponding NamedDecl is. If it isn't, mark this reference as
+// implicit. An example of implicit reference (implicit = !spelled)
+// would be a macro expansion.

nit: the comment seems stale now.



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:723
+  AllRanges.insert(end(AllRanges), begin(ImplicitRanges), end(ImplicitRanges));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(AllRanges;

nit: instead of checking all refs, I'd check implicit references explicitly, 
similar to the SpelledRefs below (just add a UnspelledSlab).


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon question-circle color=gray} clang-tidy: unknown.

{icon question-circle color=gray} clang-format: unknown.

Build artifacts 
: 
console-log.txt 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 242578.
kbobyrev added a comment.

Remove an artifact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -316,11 +316,16 @@
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+const syntax::Token *
 spelledIdentifierTouching(SourceLocation Loc,
   const syntax::TokenBuffer );
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -700,6 +700,40 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  struct $spelled[[Foo]] {
+$spelled[[Foo]]();
+~$spelled[[Foo]]();
+  };
+  $spelled[[Foo]] Variable1;
+  $implicit[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = false;
+  runSymbolCollector(Header.code(), Main.code());
+  const auto SpelledRanges = Main.ranges("spelled");
+  const auto ImplicitRanges = Main.ranges("implicit");
+  auto AllRanges = SpelledRanges;
+  AllRanges.insert(end(AllRanges), begin(ImplicitRanges), end(ImplicitRanges));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(AllRanges;
+  RefSlab::Builder SpelledSlabBuilder;
+  for (const auto  : Refs) {
+const auto Symbol = SymbolAndRefs.first;
+for (const auto  : SymbolAndRefs.second)
+  if ((Ref.Kind & RefKind::Spelled) != RefKind::Unknown)
+SpelledSlabBuilder.insert(Symbol, Ref);
+  }
+  const auto SpelledRefs = std::move(SpelledSlabBuilder).build();
+  

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon question-circle color=gray} clang-tidy: unknown.

{icon question-circle color=gray} clang-format: unknown.

Build artifacts 
: 
console-log.txt 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 242577.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Address the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolLocation.h
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -316,11 +316,16 @@
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+const syntax::Token *
 spelledIdentifierTouching(SourceLocation Loc,
   const syntax::TokenBuffer );
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -700,6 +700,40 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  struct $spelled[[Foo]] {
+$spelled[[Foo]]();
+~$spelled[[Foo]]();
+  };
+  $spelled[[Foo]] Variable1;
+  $implicit[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = false;
+  runSymbolCollector(Header.code(), Main.code());
+  const auto SpelledRanges = Main.ranges("spelled");
+  const auto ImplicitRanges = Main.ranges("implicit");
+  auto AllRanges = SpelledRanges;
+  AllRanges.insert(end(AllRanges), begin(ImplicitRanges), end(ImplicitRanges));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(AllRanges;
+  RefSlab::Builder SpelledSlabBuilder;
+  for (const auto  : Refs) {
+const auto Symbol = SymbolAndRefs.first;
+for (const auto  : SymbolAndRefs.second)
+  if ((Ref.Kind & RefKind::Spelled) != RefKind::Unknown)
+

[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(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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61851 tests passed, 6 failed 
and 780 were skipped.

  failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
  failed: lld.ELF/linkerscript/filename-spec.s

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61851 tests passed, 6 failed 
and 780 were skipped.

  failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
  failed: lld.ELF/linkerscript/filename-spec.s

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 241994.
kbobyrev added a comment.

Remove unnecessary include.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolLocation.cpp
  clang-tools-extra/clangd/index/SymbolLocation.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -316,11 +316,16 @@
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+const syntax::Token *
 spelledIdentifierTouching(SourceLocation Loc,
   const syntax::TokenBuffer );
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -100,10 +100,14 @@
 MATCHER(RefRange, "") {
   const Ref  = ::testing::get<0>(arg);
   const Range  = ::testing::get<1>(arg);
-  return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+  return toRange(Pos.Location) == Range;
+}
+MATCHER_P2(RefKindIs, Kind, Ranges, "") {
+  for (const auto  : arg)
+if ((llvm::find(Ranges, toRange(Reference.Location)) != end(Ranges)) ==
+((Reference.Kind & Kind) == RefKind::Unknown))
+  return false;
+  return true;
 }
 ::testing::Matcher &>
 HaveRanges(const std::vector Ranges) {
@@ -700,6 +704,32 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  struct $spelled[[Foo]] {
+$spelled[[Foo]]();
+

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 241993.
kbobyrev marked 14 inline comments as done.
kbobyrev added a comment.

Address a number of comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/SymbolLocation.cpp
  clang-tools-extra/clangd/index/SymbolLocation.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -316,11 +316,16 @@
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+const syntax::Token *
 spelledIdentifierTouching(SourceLocation Loc,
   const syntax::TokenBuffer );
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -100,10 +100,14 @@
 MATCHER(RefRange, "") {
   const Ref  = ::testing::get<0>(arg);
   const Range  = ::testing::get<1>(arg);
-  return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
- Pos.Location.End.line(), Pos.Location.End.column()) ==
- std::make_tuple(Range.start.line, Range.start.character,
- Range.end.line, Range.end.character);
+  return toRange(Pos.Location) == Range;
+}
+MATCHER_P2(RefKindIs, Kind, Ranges, "") {
+  for (const auto  : arg)
+if ((llvm::find(Ranges, toRange(Reference.Location)) != end(Ranges)) ==
+((Reference.Kind & Kind) == RefKind::Unknown))
+  return false;
+  return true;
 }
 ::testing::Matcher &>
 HaveRanges(const std::vector Ranges) {
@@ -700,6 +704,32 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  struct Foo;
+  #define MACRO Foo
+  

[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(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
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 

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
> spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef 
> Tokens);
> llvm::ArrayRef
> spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
> ```
> 
> 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

[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  : 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(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(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
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 

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
spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
llvm::ArrayRef
spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
```

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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61857 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61857 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 239545.
kbobyrev added a comment.

Attempt to drop collected reference before doing more computation to improve
performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -313,11 +313,22 @@
   const SourceManager *SourceMgr;
 };
 
+/// The spelled tokens that overlap or touch a spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
+/// The identifier token that overlaps or touches a spelling location Loc.
+/// If there is none, returns nullptr.
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -42,6 +42,7 @@
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::Pair;
+using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 using ::testing::UnorderedElementsAreArray;
 
@@ -700,6 +701,40 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  class Foo {};
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  $spelled[[Foo]] Variable1;
+  $macro[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.DropImplicitReferences = true;
+  runSymbolCollector(Header.code(), Main.code());
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Main.ranges("spelled");
+  EXPECT_THAT(Refs, Contains(Pair(_, HaveRanges(Main.ranges("macro");
+  EXPECT_THAT(Refs, 

[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/SymbolCollector.cpp:599
+const auto Tokens = FilesToTokensCache[MainFileID];
+for (auto  : DeclRefs) {
+  if (auto ID = getSymbolID(DeclAndRef.first)) {

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.


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 239544.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Address review comments, add implicit references filter for SymbolCollector,
test changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -313,11 +313,22 @@
   const SourceManager *SourceMgr;
 };
 
+/// The spelled tokens that overlap or touch a spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
+/// The identifier token that overlaps or touches a spelling location Loc.
+/// If there is none, returns nullptr.
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -42,6 +42,7 @@
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::Pair;
+using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 using ::testing::UnorderedElementsAreArray;
 
@@ -700,6 +701,40 @@
   EXPECT_THAT(Refs, IsEmpty());
 }
 
+TEST_F(SymbolCollectorTest, SpelledReference) {
+  Annotations Header(R"cpp(
+  class Foo {};
+  #define MACRO Foo
+  )cpp");
+  Annotations Main(R"cpp(
+  $spelled[[Foo]] Variable1;
+  $macro[[MACRO]] Variable2;
+  )cpp");
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.DropImplicitReferences = true;
+  runSymbolCollector(Header.code(), Main.code());
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Main.ranges("spelled");
+  EXPECT_THAT(Refs, Contains(Pair(_, 

[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:34
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
 };

kadircet wrote:
> hokein wrote:
> > The `All` now indicates all spelled refs. I think `All` should include both 
> > non-spelled and spell refs, which should be `declaration | Definition | 
> > Reference`.
> I don't follow the argument here ,`All` is rather a bitmask that should be 
> `OR`d with a `RefKindSet`.
> It is not like we were saying only the references that are both declarations 
> and  definitions were acceptable before, we were saying All implies either a 
> declaration, definition or a reference in here. So the logic seems to be 
> correct.
ah, you are right, I misthought about that, thanks for the clarification.


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/Ref.h:34
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
 };

hokein wrote:
> The `All` now indicates all spelled refs. I think `All` should include both 
> non-spelled and spell refs, which should be `declaration | Definition | 
> Reference`.
I don't follow the argument here ,`All` is rather a bitmask that should be 
`OR`d with a `RefKindSet`.
It is not like we were saying only the references that are both declarations 
and  definitions were acceptable before, we were saying All implies either a 
declaration, definition or a reference in here. So the logic seems to be 
correct.


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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the scope of this patch is not very clear, the changes touch two different code 
parts `SymbolCollector`, and `Rename`, and we are lacking tests for 
`SymbolCollector`. I'd suggest spliting this patch into smaller patches:

- a patch that adds a new kind to the ref, and updates the `SymbolCollector`
- a patch that updates the rename accordingly




Comment at: clang-tools-extra/clangd/index/Ref.h:33
+  Reference = 1 << 2,
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,

could you please add some brief documentation on these fields?



Comment at: clang-tools-extra/clangd/index/Ref.h:34
+  Spelled = 1 << 3,
+  All = Declaration | Definition | Reference | Spelled,
 };

The `All` now indicates all spelled refs. I think `All` should include both 
non-spelled and spell refs, which should be `declaration | Definition | 
Reference`.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:591
+  const auto MainFileID = SM.getMainFileID();
+  if (const auto MainFileURI = GetURI(MainFileID)) {
+assert(ASTCtx && "ASTContext must be set.");

looks like we don't use the `MainFileURI` variable below, I think we can remove 
this `if` statement.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:598
+  syntax::tokenize(MainFileID, SM, ASTCtx->getLangOpts());
+const auto Tokens = FilesToTokensCache[MainFileID];
+for (auto  : DeclRefs) {

since we only use `FilesToTokensCache` in this function, make it as a local 
variable rather than class member.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:599
+const auto Tokens = FilesToTokensCache[MainFileID];
+for (auto  : DeclRefs) {
+  if (auto ID = getSymbolID(DeclAndRef.first)) {

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.


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 239503.
kbobyrev added a comment.

Move added rename unit test to the end of the list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -313,11 +313,22 @@
   const SourceManager *SourceMgr;
 };
 
+/// The spelled tokens that overlap or touch a spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
+/// The identifier token that overlaps or touches a spelling location Loc.
+/// If there is none, returns nullptr.
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,7 +33,7 @@
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range , const std::string ) {
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
@@ -837,7 +837,7 @@
   {
   // variables.
   R"cpp(
-  static const int [[VA^R]] = 123;
+static const int [[VA^R]] = 123;
   )cpp",
   R"cpp(
 #include "foo.h"
@@ -868,6 +868,22 @@
 }
   )cpp",
   },
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   };
 
   for 

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 239502.
kbobyrev added a comment.

Actually, this approach might be more generic (i.e. not relying on
implementation too much). Added the FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -313,11 +313,22 @@
   const SourceManager *SourceMgr;
 };
 
+/// The spelled tokens that overlap or touch a spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
+/// The identifier token that overlaps or touches a spelling location Loc.
+/// If there is none, returns nullptr.
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,7 +33,7 @@
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range , const std::string ) {
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
@@ -728,6 +728,22 @@
 llvm::StringRef FooH;
 llvm::StringRef FooCC;
   } Cases[] = {
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   {
   // classes.
   R"cpp(
@@ -837,7 +853,7 @@
   {
   // variables.
   R"cpp(
-  

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Will put a TODO and revert helpers back to use plain binary search over the 
tokens.


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 239498.
kbobyrev added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -256,21 +256,27 @@
 
 llvm::ArrayRef
 syntax::spelledTokensTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   assert(Loc.isFileID());
-  llvm::ArrayRef All =
-  Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc));
   auto *Right = llvm::partition_point(
-  All, [&](const syntax::Token ) { return Tok.location() < Loc; });
-  bool AcceptRight = Right != All.end() && Right->location() <= Loc;
-  bool AcceptLeft = Right != All.begin() && (Right - 1)->endLocation() >= Loc;
+  Tokens, [&](const syntax::Token ) { return Tok.location() < Loc; });
+  bool AcceptRight = Right != Tokens.end() && Right->location() <= Loc;
+  bool AcceptLeft =
+  Right != Tokens.begin() && (Right - 1)->endLocation() >= Loc;
   return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
 Right + (AcceptRight ? 1 : 0));
 }
 
+llvm::ArrayRef
+syntax::spelledTokensTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledTokensTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 const syntax::Token *
 syntax::spelledIdentifierTouching(SourceLocation Loc,
-  const syntax::TokenBuffer ) {
+  llvm::ArrayRef Tokens) {
   for (const syntax::Token  : spelledTokensTouching(Loc, Tokens)) {
 if (Tok.kind() == tok::identifier)
   return 
@@ -278,6 +284,13 @@
   return nullptr;
 }
 
+const syntax::Token *
+syntax::spelledIdentifierTouching(SourceLocation Loc,
+  const syntax::TokenBuffer ) {
+  return spelledIdentifierTouching(
+  Loc, Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)));
+}
+
 std::vector
 TokenBuffer::macroExpansions(FileID FID) const {
   auto FileIt = Files.find(FID);
Index: clang/include/clang/Tooling/Syntax/Tokens.h
===
--- clang/include/clang/Tooling/Syntax/Tokens.h
+++ clang/include/clang/Tooling/Syntax/Tokens.h
@@ -313,11 +313,22 @@
   const SourceManager *SourceMgr;
 };
 
+/// The spelled tokens that overlap or touch a spelling location Loc.
+/// This always returns 0-2 tokens.
+llvm::ArrayRef
+spelledTokensTouching(SourceLocation Loc, llvm::ArrayRef Tokens);
+
 /// The spelled tokens that overlap or touch a spelling location Loc.
 /// This always returns 0-2 tokens.
 llvm::ArrayRef
 spelledTokensTouching(SourceLocation Loc, const syntax::TokenBuffer );
 
+/// The identifier token that overlaps or touches a spelling location Loc.
+/// If there is none, returns nullptr.
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+  llvm::ArrayRef Tokens);
+
 /// The identifier token that overlaps or touches a spelling location Loc.
 /// If there is none, returns nullptr.
 const syntax::Token *
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,7 +33,7 @@
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range , const std::string ) {
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
@@ -728,6 +728,22 @@
 llvm::StringRef FooH;
 llvm::StringRef FooCC;
   } Cases[] = {
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   {
   // classes.
   R"cpp(
@@ -837,7 +853,7 @@
   {
   // variables.
   R"cpp(
-  static const int [[VA^R]] = 123;
+static const int [[VA^R]] = 123;
   )cpp",
   

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72746#1827608 , @kbobyrev wrote:

> I started using TokenBuffer, but I ran into the following issue: when I'm
>  creating `TokenBuffer` and `TokenCollector`, they do not contain any tokens.
>  `Preprocessor` does not seem to have a non-null Lexer instance, 
> `TokenWatcher`
>  (set in `TokenCollector`) is not triggered anywhere and neither is
>  `Lexer::Lex`. I don't have much familiarity with the code and I looked at the
>  other usages of `TokenBuffer` but didn't figure out what's wrong with the 
> code
>  in this patch. I suspect the lexer in Preprocessor should be re-initialized
>  somehow? I'm certainly missing something here.


right, I forgot that `TokenCollector` must be constructed before starting to 
process the file, and unfortunately in some
code paths we create a `SymbolCollector` long after parsing the file. You can 
make use of `syntax::Tokenize` for now,
as we only care about SpelledTokens in the file. Please leave a TODO though 
saying that, we should rather pass one from
the caller of SymbolCollector.

Sorry for the inconvenience.




Comment at: clang-tools-extra/clangd/index/Ref.h:36
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,

kbobyrev wrote:
> kadircet wrote:
> > kadircet wrote:
> > > instead of doing that, could we rather de-couple two enums completely and 
> > > have a `symbolRoleToRefKind`(and vice-versa) method instead(we already 
> > > have some customization in `toRefKind` now) ?
> > > 
> > > as current change increases the risk of overlapping in future(e.g. 
> > > someone might change symbolrole::declaration and cause 
> > > failures/regressions in clangd)
> > note that this would also require a bump to version of `on-disk` index in 
> > clangd/index/Serialization.cpp, as old information regarding `RefKind` 
> > won't be usable anymore.
> > instead of doing that, could we rather de-couple two enums completely and 
> > have a symbolRoleToRefKind(and vice-versa) method instead(we already have 
> > some customization in toRefKind now) ?
> > as current change increases the risk of overlapping in future(e.g. someone 
> > might change symbolrole::declaration and cause failures/regressions in 
> > clangd)
> 
> Makes sense, will do!
> 
> > note that this would also require a bump to version of on-disk index in 
> > clangd/index/Serialization.cpp, as old information regarding RefKind won't 
> > be usable anymore.
> 
> I checked the Serialization code and the serialization code should be OK as 
> long as `RefKind` stays one byte. Can you please elaborate on this?
> 
> Do you mean that the index should be generated again after this change 
> because it would no longer be valid? (this one I understand)
> Or do you mean there should be some other change in the code for me to do to 
> land this patch?
> Do you mean that the index should be generated again after this change 
> because it would no longer be valid? (this one I understand)

yes i meant this


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61674 tests passed, 181 failed 
and 781 were skipped.

  failed: Clangd.Clangd/background-index.test
  failed: Clangd.Clangd/code-action-request.test
  failed: Clangd.Clangd/compile-commands-path-in-initialize.test
  failed: Clangd.Clangd/completion-auto-trigger.test
  failed: Clangd.Clangd/completion-snippets.test
  failed: Clangd.Clangd/completion.test
  failed: Clangd.Clangd/diagnostic-category.test
  failed: Clangd.Clangd/diagnostics-no-tidy.test
  failed: Clangd.Clangd/diagnostics-notes.test
  failed: Clangd.Clangd/diagnostics.test
  failed: Clangd.Clangd/did-change-configuration-params.test
  failed: Clangd.Clangd/document-link.test
  failed: Clangd.Clangd/execute-command.test
  failed: Clangd.Clangd/filestatus.test
  failed: Clangd.Clangd/fixits-codeaction.test
  failed: Clangd.Clangd/fixits-command.test
  failed: Clangd.Clangd/fixits-embed-in-diagnostic.test
  failed: Clangd.Clangd/formatting.test
  failed: Clangd.Clangd/hover.test
  failed: Clangd.Clangd/index-tools.test
  failed: Clangd.Clangd/path-mappings.test
  failed: Clangd.Clangd/protocol.test
  failed: Clangd.Clangd/references.test
  failed: Clangd.Clangd/rename.test
  failed: Clangd.Clangd/request-reply.test
  failed: Clangd.Clangd/selection-range.test
  failed: Clangd.Clangd/semantic-highlighting.test
  failed: Clangd.Clangd/signature-help-with-offsets.test
  failed: Clangd.Clangd/signature-help.test
  failed: Clangd.Clangd/symbol-info.test
  failed: Clangd.Clangd/symbols.test
  failed: Clangd.Clangd/system-include-extractor.test
  failed: Clangd.Clangd/test-uri-posix.test
  failed: Clangd.Clangd/textdocument-didchange-fail.test
  failed: Clangd.Clangd/trace.test
  failed: Clangd.Clangd/tweaks-format.test
  failed: Clangd.Clangd/type-hierarchy.test
  failed: Clangd.Clangd/unsupported-method.test
  failed: Clangd.Clangd/utf8.test
  failed: Clangd.Clangd/xrefs.test
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.CmdLineHash
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.DirectIncludesTest
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.IndexTwoFiles
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoCrashOnErrorFile
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.NoDotsInAbsPath
  failed: Clangd Unit 
Tests._/ClangdTests/BackgroundIndexTest.ShardStorageEmptyFile
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageLoad
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.ShardStorageTest
  failed: Clangd Unit Tests._/ClangdTests/BackgroundIndexTest.UncompilableFiles
  failed: Clangd Unit 
Tests._/ClangdTests/CompletionTest.CommentsFromSystemHeaders
  failed: Clangd Unit 
Tests._/ClangdTests/CompletionTest.DynamicIndexIncludeInsertion
  failed: Clangd Unit Tests._/ClangdTests/CompletionTest.DynamicIndexMultiFile
  failed: Clangd Unit Tests._/ClangdTests/CompletionTest.UsingDecl
  failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.DirtyBuffer
  failed: Clangd Unit Tests._/ClangdTests/CrossFileRenameTests.WithUpToDateIndex
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.BasicSymbols
  failed: Clangd Unit 
Tests._/ClangdTests/DocumentSymbolsTest.DeclarationDefinition
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Enums
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.ExternSymbol
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FromMacro
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.FuncTemplates
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.InHeaderFile
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Namespaces
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.NoLocals
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Qualifiers
  failed: Clangd Unit 
Tests._/ClangdTests/DocumentSymbolsTest.QualifiersWithTemplateArgs
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.TempSpecs
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Template
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.Unnamed
  failed: Clangd Unit Tests._/ClangdTests/DocumentSymbolsTest.UsingDirectives
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.ClassMembers
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CollectMacros
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.CustomizedURIScheme
  failed: Clangd Unit 
Tests._/ClangdTests/FileIndexTest.HasSystemHeaderMappingsInPreamble
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IncludeCollected
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.IndexAST
  failed: Clangd Unit 
Tests._/ClangdTests/FileIndexTest.IndexMultiASTAndDeduplicate
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MacroRefs
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.MergeMainFileSymbols
  failed: Clangd Unit Tests._/ClangdTests/FileIndexTest.NoLocal
  

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 238902.
kbobyrev added a comment.

I started using TokenBuffer, but I ran into the following issue: when I'm
creating `TokenBuffer` and `TokenCollector`, they do not contain any tokens.
`Preprocessor` does not seem to have a non-null Lexer instance, `TokenWatcher`
(set in `TokenCollector`) is not triggered anywhere and neither is
`Lexer::Lex`. I don't have much familiarity with the code and I looked at the
other usages of `TokenBuffer` but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,7 +33,7 @@
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range , const std::string ) {
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
@@ -837,7 +837,7 @@
   {
   // variables.
   R"cpp(
-  static const int [[VA^R]] = 123;
+static const int [[VA^R]] = 123;
   )cpp",
   R"cpp(
 #include "foo.h"
@@ -868,6 +868,22 @@
 }
   )cpp",
   },
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   };
 
   for (const auto& T : Cases) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -296,6 +296,8 @@
   bool HasMore = Index.refs(RQuest, [&](const Ref ) {
 if (AffectedFiles.size() > MaxLimitFiles)
   return;
+if (!static_cast(R.Kind & RefKind::Spelled))
+  return;
 if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
   if (*RefFilePath != MainFile)
 AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -28,6 +28,7 @@
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -180,7 +181,16 @@
 }
 
 RefKind toRefKind(index::SymbolRoleSet Roles) {
-  return static_cast(static_cast(RefKind::All) & Roles);
+  RefKind Result = RefKind::Unknown;
+  if (Roles & static_cast(index::SymbolRole::Declaration))
+Result |= RefKind::Declaration;
+  if (Roles & static_cast(index::SymbolRole::Definition))
+Result |= RefKind::Definition;
+  if (Roles & static_cast(index::SymbolRole::Reference))
+Result |= RefKind::Reference;
+  if (!(Roles & static_cast(index::SymbolRole::Implicit)))
+Result |= RefKind::Spelled;
+  return Result;
 }
 
 bool shouldIndexRelation(const index::SymbolRelation ) {
@@ -291,7 +301,7 @@
   // occurrence inside the base-specifier.
   processRelations(*ND, *ID, Relations);
 
-  bool CollectRef = static_cast(Opts.RefFilter) & Roles;
+  bool CollectRef = static_cast(Opts.RefFilter & toRefKind(Roles));
   bool IsOnlyRef =
   !(Roles & (static_cast(index::SymbolRole::Declaration) |
  static_cast(index::SymbolRole::Definition)));
@@ -578,10 +588,25 @@
   }
   // Populate Refs slab from DeclRefs.
   if (auto MainFileURI = GetURI(SM.getMainFileID())) {
-for (const auto  : DeclRefs) {
-  if (auto ID = getSymbolID(It.first)) {
-for (const auto  : It.second)
+assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
+syntax::TokenCollector CollectTokens(*PP);
+syntax::TokenBuffer Tokens = std::move(CollectTokens).consume();
+for (auto  : DeclRefs) {
+  if (auto ID = getSymbolID(DeclAndRef.first)) {
+for (auto  : DeclAndRef.second) {
+  // Check if the 

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Need to make use of the `TokenBuffer`, ran into some difficulties when lexing 
some files and triggering assertions in the process. Will figure it out and 
update the patch.


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 238786.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.

Address alsmost all comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,7 +33,7 @@
 // Convert a Range to a Ref.
 Ref refWithRange(const clangd::Range , const std::string ) {
   Ref Result;
-  Result.Kind = RefKind::Reference;
+  Result.Kind = RefKind::Reference | RefKind::Spelled;
   Result.Location.Start.setLine(Range.start.line);
   Result.Location.Start.setColumn(Range.start.character);
   Result.Location.End.setLine(Range.end.line);
@@ -837,7 +837,7 @@
   {
   // variables.
   R"cpp(
-  static const int [[VA^R]] = 123;
+static const int [[VA^R]] = 123;
   )cpp",
   R"cpp(
 #include "foo.h"
@@ -868,6 +868,22 @@
 }
   )cpp",
   },
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   };
 
   for (const auto& T : Cases) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -296,6 +296,8 @@
   bool HasMore = Index.refs(RQuest, [&](const Ref ) {
 if (AffectedFiles.size() > MaxLimitFiles)
   return;
+if (!static_cast(R.Kind & RefKind::Spelled))
+  return;
 if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
   if (*RefFilePath != MainFile)
 AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -28,6 +28,7 @@
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -180,7 +181,16 @@
 }
 
 RefKind toRefKind(index::SymbolRoleSet Roles) {
-  return static_cast(static_cast(RefKind::All) & Roles);
+  RefKind Result = RefKind::Unknown;
+  if (Roles & static_cast(index::SymbolRole::Declaration))
+Result |= RefKind::Declaration;
+  if (Roles & static_cast(index::SymbolRole::Definition))
+Result |= RefKind::Definition;
+  if (Roles & static_cast(index::SymbolRole::Reference))
+Result |= RefKind::Reference;
+  if (!(Roles & static_cast(index::SymbolRole::Implicit)))
+Result |= RefKind::Spelled;
+  return Result;
 }
 
 bool shouldIndexRelation(const index::SymbolRelation ) {
@@ -291,7 +301,7 @@
   // occurrence inside the base-specifier.
   processRelations(*ND, *ID, Relations);
 
-  bool CollectRef = static_cast(Opts.RefFilter) & Roles;
+  bool CollectRef = static_cast(Opts.RefFilter & toRefKind(Roles));
   bool IsOnlyRef =
   !(Roles & (static_cast(index::SymbolRole::Declaration) |
  static_cast(index::SymbolRole::Definition)));
@@ -578,10 +588,22 @@
   }
   // Populate Refs slab from DeclRefs.
   if (auto MainFileURI = GetURI(SM.getMainFileID())) {
-for (const auto  : DeclRefs) {
-  if (auto ID = getSymbolID(It.first)) {
-for (const auto  : It.second)
+for (auto  : DeclRefs) {
+  if (auto ID = getSymbolID(DeclAndRef.first)) {
+for (auto  : DeclAndRef.second) {
+  // Check if the referenced symbol is spelled exactly the same way the
+  // corresponding NamedDecl is. If it isn't, mark this reference as
+  // implicit.  An example of implicit references would be a macro
+  // expansion.
+  llvm::SmallString<16> Buffer;
+  const auto Spelling = Lexer::getSpelling(LocAndRole.first, Buffer, SM,
+   ASTCtx->getLangOpts());
+  DeclarationName Name = DeclAndRef.first->getDeclName();
+  if (Name.isIdentifier() && Name.getAsString() != Spelling)
+LocAndRole.second |=
+static_cast(index::SymbolRole::Implicit);
   CollectRef(*ID, LocAndRole);
+ 

[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:36
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,

kadircet wrote:
> kadircet wrote:
> > instead of doing that, could we rather de-couple two enums completely and 
> > have a `symbolRoleToRefKind`(and vice-versa) method instead(we already have 
> > some customization in `toRefKind` now) ?
> > 
> > as current change increases the risk of overlapping in future(e.g. someone 
> > might change symbolrole::declaration and cause failures/regressions in 
> > clangd)
> note that this would also require a bump to version of `on-disk` index in 
> clangd/index/Serialization.cpp, as old information regarding `RefKind` won't 
> be usable anymore.
> instead of doing that, could we rather de-couple two enums completely and 
> have a symbolRoleToRefKind(and vice-versa) method instead(we already have 
> some customization in toRefKind now) ?
> as current change increases the risk of overlapping in future(e.g. someone 
> might change symbolrole::declaration and cause failures/regressions in clangd)

Makes sense, will do!

> note that this would also require a bump to version of on-disk index in 
> clangd/index/Serialization.cpp, as old information regarding RefKind won't be 
> usable anymore.

I checked the Serialization code and the serialization code should be OK as 
long as `RefKind` stays one byte. Can you please elaborate on this?

Do you mean that the index should be generated again after this change because 
it would no longer be valid? (this one I understand)
Or do you mean there should be some other change in the code for me to do to 
land this patch?


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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D72746#1821097 , @kbobyrev wrote:

> The patch could be shorter and slightly less confusing if I preserved 1:1 
> `RefKind::Implicit` <-> `index::SymbolKind::Implicit` relation, but I thought 
> we might want to keep `RefKind` being 1 byte so that we don't waste 
> unnecessary memory.


I think it is time to create our own `Declaration`, `Definition` in our 
`RefKind` (rather than reusing the enums in libindex).

> Also, since we might want to start using `findExplicitReferences` (or 
> something else we come up with) for indexing, it shouldn't be critical. Let 
> me know if you think it's unnecessary.

Yeah, we may have a plan in the future, but I don't think switching to 
`findExplicitReferences` is trivial, there are a few things we need to figure 
out carefully, e.g. how to support macros, implicit references.




Comment at: clang-tools-extra/clangd/index/Ref.h:33
   Reference = static_cast(index::SymbolRole::Reference),
-  All = Declaration | Definition | Reference,
+  // This corresponds to index::SymbolRole::Implicit. In order to save memory
+  // and keep RefKind occupying 1 byte, the original value is modified to the

I'm skeptical about definition of `index::SymbolRole::Implicit`.

I think the `implicit` we want here is references that are spelled/written as 
the same as the named decls, so it is more likely to align with 
`index::SymbolRole::NameReferences`.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:309
+  if (Name.isIdentifier() && Name.getAsString() != Spelling)
+Roles |= static_cast(index::SymbolRole::Implicit);
+

kadircet wrote:
> It seems like we are only checking for `syntactic` occurence of a symbol. Can 
> we make use of `Spelled` Rather than `Implicit` (which has some semantic 
> meaning) to make that clear?
+1, `Implicit` is ambiguous, maybe `Named`, `Explicit` (this reflects to the 
`findExplicitReferences`)?


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/Ref.h:36
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,

kadircet wrote:
> instead of doing that, could we rather de-couple two enums completely and 
> have a `symbolRoleToRefKind`(and vice-versa) method instead(we already have 
> some customization in `toRefKind` now) ?
> 
> as current change increases the risk of overlapping in future(e.g. someone 
> might change symbolrole::declaration and cause failures/regressions in clangd)
note that this would also require a bump to version of `on-disk` index in 
clangd/index/Serialization.cpp, as old information regarding `RefKind` won't be 
usable anymore.


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/Ref.h:36
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,

instead of doing that, could we rather de-couple two enums completely and have 
a `symbolRoleToRefKind`(and vice-versa) method instead(we already have some 
customization in `toRefKind` now) ?

as current change increases the risk of overlapping in future(e.g. someone 
might change symbolrole::declaration and cause failures/regressions in clangd)



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:306
+  const auto Spelling =
+  Lexer::getSpelling(Loc, Buffer, SM, ASTCtx->getLangOpts());
+  DeclarationName Name = ND->getDeclName();

this might be expensive to trigger while indexing for each reference.

could we rather do it as post-processing inside `SymbolCollector::finish` while 
making use of `TokenBuffer::spelledTokens`(you can create a tokenbuffer through 
`TokenCollector` with the `PP` inside `SymbolCollector`.)



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:309
+  if (Name.isIdentifier() && Name.getAsString() != Spelling)
+Roles |= static_cast(index::SymbolRole::Implicit);
+

It seems like we are only checking for `syntactic` occurence of a symbol. Can 
we make use of `Spelled` Rather than `Implicit` (which has some semantic 
meaning) to make that clear?


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

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

The patch could be shorter and slightly less confusing if I preserved 1:1 
`RefKind::Implicit` <-> `index::SymbolKind::Implicit` relation, but I thought 
we might want to keep `RefKind` being 1 byte so that we don't waste unnecessary 
memory. Also, since we might want to start using `findExplicitReferences` (or 
something else we come up with) for indexing, it shouldn't be critical. Let me 
know if you think it's unnecessary.

+CC @hokein


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

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.
kbobyrev added a subscriber: hokein.

This patch allows the index does to provide a way to distinguish implicit
references (e.g. coming from macro expansions). The corresponding flag was
added to RefKind and symbols that are referenced without spelling their name
explicitly are now marked implicit. This fixes incorrect behavior when renaming
a symbol that was referenced in macro expansions would try to rename macro
invocations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72746

Files:
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -837,7 +837,7 @@
   {
   // variables.
   R"cpp(
-  static const int [[VA^R]] = 123;
+static const int [[VA^R]] = 123;
   )cpp",
   R"cpp(
 #include "foo.h"
@@ -868,6 +868,22 @@
 }
   )cpp",
   },
+  {
+  // Implicit references in macro expansions.
+  R"cpp(
+class [[Fo^o]] {};
+#define FooFoo Foo
+#define FOO Foo
+  )cpp",
+  R"cpp(
+#include "foo.h"
+void bar() {
+  [[Foo]] x;
+  FOO y;
+  FooFoo z;
+}
+  )cpp",
+  },
   };
 
   for (const auto& T : Cases) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -296,6 +296,8 @@
   bool HasMore = Index.refs(RQuest, [&](const Ref ) {
 if (AffectedFiles.size() > MaxLimitFiles)
   return;
+if (static_cast(R.Kind & RefKind::Implicit))
+  return;
 if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
   if (*RefFilePath != MainFile)
 AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -180,7 +180,14 @@
 }
 
 RefKind toRefKind(index::SymbolRoleSet Roles) {
-  return static_cast(static_cast(RefKind::All) & Roles);
+  // Because RefKind::Implicit != index::SymbolRole::Implicit because due to the
+  // memory considerations, all flags except "Implicit" should be set
+  // independently.
+  auto Result =
+  static_cast(static_cast(RefKind::NotImplicit) & Roles);
+  if (Roles & static_cast(index::SymbolRole::Implicit))
+Result |= RefKind::Implicit;
+  return Result;
 }
 
 bool shouldIndexRelation(const index::SymbolRelation ) {
@@ -291,7 +298,17 @@
   // occurrence inside the base-specifier.
   processRelations(*ND, *ID, Relations);
 
-  bool CollectRef = static_cast(Opts.RefFilter) & Roles;
+  // Check if the referenced symbol is spelled exactly the same way the
+  // corresponding NamedDecl is. If it isn't, mark this reference as implicit.
+  // An example of implicit references would be a macro expansion.
+  llvm::SmallString<16> Buffer;
+  const auto Spelling =
+  Lexer::getSpelling(Loc, Buffer, SM, ASTCtx->getLangOpts());
+  DeclarationName Name = ND->getDeclName();
+  if (Name.isIdentifier() && Name.getAsString() != Spelling)
+Roles |= static_cast(index::SymbolRole::Implicit);
+
+  bool CollectRef = static_cast(Opts.RefFilter & toRefKind(Roles));
   bool IsOnlyRef =
   !(Roles & (static_cast(index::SymbolRole::Declaration) |
  static_cast(index::SymbolRole::Definition)));
Index: clang-tools-extra/clangd/index/Ref.h
===
--- clang-tools-extra/clangd/index/Ref.h
+++ clang-tools-extra/clangd/index/Ref.h
@@ -30,7 +30,12 @@
   Declaration = static_cast(index::SymbolRole::Declaration),
   Definition = static_cast(index::SymbolRole::Definition),
   Reference = static_cast(index::SymbolRole::Reference),
-  All = Declaration | Definition | Reference,
+  // This corresponds to index::SymbolRole::Implicit. In order to save memory
+  // and keep RefKind occupying 1 byte, the original value is modified to the
+  // one below.
+  Implicit = 1 << 3,
+  All = Declaration | Definition | Reference | Implicit,
+  NotImplicit = Declaration | Definition | Reference,
 };
 
 inline RefKind operator|(RefKind L, RefKind R) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org