[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366070: [clangd] Fix duplicate highlighting tokens appearing 
in initializer lists. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64634?vs=209852&id=209868#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
  clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &L, const HighlightingToken &R) {
+ return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 


Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &L, const HighlightingToken &R) {
+ return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM (I think @hokein 's comments were addressed too)

> So it seems to be expected. (and looking at the documentation for 
> InitListExpr it seems to be difficult to change RecursiveASTVisitor to visit 
> every sub expr once)

Ah, this particular piece of behavior seems really surprising to me.
We should definitely only traverse syntactic form if `shouldVisitImplicitCode() 
== true`, which should help us avoid this corner case here. But that's a topic 
for a separate patch, please land this to unbreak semantic highlighting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634



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

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added a comment.

In D64634#1585521 , @ilya-biryukov 
wrote:

> Have we tried figuring out why `RecursiveASTVisitor` visits the argument 
> lists twice? Is that an expected behavior?


The comment for the function that traverses initialization lists says this:

> // This method is called once for each pair of syntactic and semantic
>  // InitListExpr, and it traverses the subtrees defined by the two forms. This
>  // may cause some of the children to be visited twice, if they appear both in
>  // the syntactic and the semantic form.

So it seems to be expected. (and looking at the documentation for InitListExpr 
it seems to be difficult to change RecursiveASTVisitor to visit every sub expr 
once)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634



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

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 209852.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.

Address comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &L, const HighlightingToken &R) {
+ return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &L, const HighlightingToken &R) {
+ return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Have we tried figuring out why `RecursiveASTVisitor` visits the argument lists 
twice? Is that an expected behavior?




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:37
+llvm::sort(Tokens,
+   [](const HighlightingToken &Lhs, const HighlightingToken &Rhs) {
+ return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);

NIT: in LLVM style, these should be called `LHS` and `RHS` as they are 
abbreviations.
However, in clangd we typically use `L` and `R` for that in those cases, I 
would suggest sticking to that.

(One-letter names are harder to confuse with each other)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634



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

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 209775.
jvikstrom added a comment.

Removed return type hint for lambda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &Lhs, const HighlightingToken &Rhs) {
+ return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,14 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(Tokens,
+   [](const HighlightingToken &Lhs, const HighlightingToken &Rhs) {
+ return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+   });
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 209774.
jvikstrom added a comment.

Readded newline that was removed accidentaly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,15 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(
+Tokens,
+[](const HighlightingToken &Lhs, const HighlightingToken &Rhs) -> bool 
{
+  return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+});
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,15 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(
+Tokens,
+[](const HighlightingToken &Lhs, const HighlightingToken &Rhs) -> bool {
+  return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+});
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 209773.
jvikstrom added a comment.

Remove operator< and use std::unique.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,15 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(
+Tokens,
+[](const HighlightingToken &Lhs, const HighlightingToken &Rhs) -> bool 
{
+  return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+});
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
@@ -80,7 +89,6 @@
 DeclarationName::Identifier)
   // Only want to highlight identifiers.
   return true;
-
 addToken(Ref->getLocation(), Ref->getDecl());
 return true;
   }


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -166,6 +166,13 @@
 $Variable[[AA]].$Field[[E]].$Field[[C]];
 $Class[[A]]::$Variable[[S]] = 90;
   }
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int $Field[[A]];
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,15 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+llvm::sort(
+Tokens,
+[](const HighlightingToken &Lhs, const HighlightingToken &Rhs) -> bool {
+  return std::tie(Lhs.R, Lhs.Kind) < std::tie(Rhs.R, Rhs.Kind);
+});
+auto Last = std::unique(Tokens.begin(), Tokens.end());
+Tokens.erase(Last, Tokens.end());
 return Tokens;
   }
 
@@ -80,7 +89,6 @@
 DeclarationName::Identifier)
   // Only want to highlight identifiers.
   return true;
-
 addToken(Ref->getLocation(), Ref->getDecl());
 return true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:36
+// must be deduplicated.
+std::sort(Tokens.begin(), Tokens.end());
+for (unsigned I = 1; I < Tokens.size(); ++I) {

hokein wrote:
> nit: we could write it like
> 
> ```
> llvm::sort(Tokens, [](const HighlightingToken &Lhs, const HighlightingToken 
> &Rhs) {
>return std::tie(Lhs.Kind, Lhs.R) < std::tie(Rhs.Kind, Rhs.R);
> });
> auto Last = std::unique(Tokens.begin(), Tokens.end());
> Tokens.erase(Last, Tokens.end());
> ```
About the comparator function though. I added an < operator because 
https://reviews.llvm.org/D64475 also sorts the tokens. However, could I just 
remove the sort in the main diffHighlightings method and just rely on the 
highlightings to be sorted in the first place from this patch?

Instead we'd do the sorting like:
```
llvm::sort(Tokens, [](const HighlightingToken &Lhs, const HighlightingToken 
&Rhs) {
   return std::tie(Lhs.R,Lhs.Kind) < std::tie(Rhs.R,Rhs.Kind);
});
```

so we get it sorted by their position first and Kind second.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634



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

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:36
+// must be deduplicated.
+std::sort(Tokens.begin(), Tokens.end());
+for (unsigned I = 1; I < Tokens.size(); ++I) {

nit: we could write it like

```
llvm::sort(Tokens, [](const HighlightingToken &Lhs, const HighlightingToken 
&Rhs) {
   return std::tie(Lhs.Kind, Lhs.R) < std::tie(Rhs.Kind, Rhs.R);
});
auto Last = std::unique(Tokens.begin(), Tokens.end());
Tokens.erase(Last, Tokens.end());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64634



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

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom created this revision.
jvikstrom added reviewers: hokein, sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, 
MaskRay.
Herald added a project: clang.

The RecursiveASTVisitor sometimes visits exprs in initializer lists twice. 
Added deduplication to prevent duplicate highlighting tokens from appearing. 
Done by sorting and a linear search.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64634

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -132,6 +132,13 @@
 $Namespace[[vwz]]::$Class[[A]]::$Enum[[B]]::Hi;
   ::$Namespace[[vwz]]::$Class[[A]] $Variable[[B]];
   ::$Namespace[[abc]]::$Namespace[[bcd]]::$Class[[A]] $Variable[[BB]];
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int A;
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -40,6 +40,7 @@
 };
 
 bool operator==(const HighlightingToken &Lhs, const HighlightingToken &Rhs);
+bool operator<(const HighlightingToken &Lhs, const HighlightingToken &Rhs);
 
 // Returns all HighlightingTokens from an AST. Only generates highlights for 
the
 // main AST.
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,16 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+std::sort(Tokens.begin(), Tokens.end());
+for (unsigned I = 1; I < Tokens.size(); ++I) {
+  if (Tokens[I] == Tokens[I - 1]) {
+Tokens.erase(Tokens.begin() + I);
+--I;
+  }
+}
+
 return Tokens;
   }
 
@@ -70,7 +80,6 @@
 DeclarationName::Identifier)
   // Only want to highlight identifiers.
   return true;
-
 addToken(Ref->getLocation(), Ref->getDecl());
 return true;
   }
@@ -201,6 +210,10 @@
   return Lhs.Kind == Rhs.Kind && Lhs.R == Rhs.R;
 }
 
+bool operator<(const HighlightingToken &Lhs, const HighlightingToken &Rhs) {
+  return Lhs.R.start < Rhs.R.start;
+}
+
 std::vector getSemanticHighlightings(ParsedAST &AST) {
   return HighlightingTokenCollector(AST).collectTokens();
 }


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -132,6 +132,13 @@
 $Namespace[[vwz]]::$Class[[A]]::$Enum[[B]]::Hi;
   ::$Namespace[[vwz]]::$Class[[A]] $Variable[[B]];
   ::$Namespace[[abc]]::$Namespace[[bcd]]::$Class[[A]] $Variable[[BB]];
+)cpp",
+R"cpp(
+  struct $Class[[AA]] {
+int A;
+  }
+  int $Variable[[B]];
+  $Class[[AA]] $Variable[[A]]{$Variable[[B]]};
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -40,6 +40,7 @@
 };
 
 bool operator==(const HighlightingToken &Lhs, const HighlightingToken &Rhs);
+bool operator<(const HighlightingToken &Lhs, const HighlightingToken &Rhs);
 
 // Returns all HighlightingTokens from an AST. Only generates highlights for the
 // main AST.
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -31,6 +31,16 @@
   std::vector collectTokens() {
 Tokens.clear();
 TraverseAST(Ctx);
+// Initializer lists can give duplicates of tokens, therefore all tokens
+// must be deduplicated.
+std::sort(Tokens.begin(), Tokens.end());
+for (unsigned I = 1; I < Tokens.size(); ++I) {
+  if (Tokens[I] == Tokens[I - 1]) {
+Tokens.erase(Tokens.begin() + I);
+--I;
+  }
+}
+
 retur