[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367521: [clangd] Duplicate lines of semantic highlightings 
sent removed. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64475?vs=212528=212747#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
  clang-tools-extra/trunk/clangd/SemanticHighlighting.h
  clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
  clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
===
--- clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
+++ clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
@@ -49,6 +49,50 @@
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo2.cpp","languageId":"cpp","version":1,"text":"int x = 2;\nint y = 2;"}}}
+#  CHECK:  "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:  }
+# CHECK-NEXT:  {
+# CHECK-NEXT:"line": 1,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT:  "uri": "file:///clangd-test/foo2.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.cpp","version":2},"contentChanges": [{"range":{"start": {"line": 0,"character": 10},"end": {"line": 0,"character": 10}},"rangeLength": 0,"text": "\nint y = 2;"}]}}
+#  CHECK:  "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"line": 1,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT:  }
+# CHECK-NEXT:   ],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT:  "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.cpp","version":2},"contentChanges": [{"range":{"start": {"line": 0,"character": 10},"end": {"line": 1,"character": 10}},"rangeLength": 11,"text": ""}]}}
+#  CHECK:  "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"lines": [],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT:  "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,10 +46,47 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens))
-   << "Inputs is:\n" << Code;
+// Any annotations in OldCode and NewCode are converted into their corresponding
+// HighlightingToken. The tokens are diffed against each other. Any lines where
+// the tokens should diff must be marked with a ^ somewhere on that line in
+// NewCode. If there are diffs that aren't marked with ^ the test fails. The
+// test also fails if there are lines marked with ^ that don't differ.
+void checkDiffedHighlights(llvm::StringRef 

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+Old = std::move(FileToHighlightings[File]);
+FileToHighlightings[File] = Highlightings;
+  }

ilya-biryukov wrote:
> jvikstrom wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > NIT: avoid copying (from `Highlightings` into the map) under a lock, 
> > > > make the copy outside the lock instead
> > > I think we can even avoid copy, since we only use the `Highlightings` for 
> > > calculating the diff.
> > > 
> > > Maybe just 
> > > 
> > > ```
> > > {
> > > std::lock_guard Lock(HighlightingsMutex);
> > > Old = std::move(FileToHighlightings[File]);
> > > }
> > > // calculate the diff.
> > > {  
> > >  std::lock_guard Lock(HighlightingsMutex);
> > >  FileToHighlightings[File] = std::move(Highlightings);
> > > }
> > > ```
> > I think I changed this to only lock once (and copy instead) at the same 
> > time me and @ilya-biryukov were talking about the race condition (?)
> It means there's a window where nobody can access the highlightings for this 
> file. Which is probably fine, we really do use this only for computing the 
> diff and nothing else.
> 
> But doing the copy shouldn't matter here either, so leaving as is also looks 
> ok from my side.
> 
> If you decide to avoid an extra copy, please add comments to 
> `FileToHighlightings` (e.g. `only used to compute diffs, must not be used 
> outside onHighlightingsReady and didRemove`).
> It is declared really close to `FixItsMap` and looks very similar, but the 
> latter is used in completely different ways; that could lead to confusion.
> 
> Don't remember exact details about race conditions, but we should be good now 
> that it's called inside `Publish()`
I'm fine with the current state, avoiding the copy cost would make the code 
here a bit mess...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

LGTM from my side




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+Old = std::move(FileToHighlightings[File]);
+FileToHighlightings[File] = Highlightings;
+  }

jvikstrom wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > NIT: avoid copying (from `Highlightings` into the map) under a lock, make 
> > > the copy outside the lock instead
> > I think we can even avoid copy, since we only use the `Highlightings` for 
> > calculating the diff.
> > 
> > Maybe just 
> > 
> > ```
> > {
> > std::lock_guard Lock(HighlightingsMutex);
> > Old = std::move(FileToHighlightings[File]);
> > }
> > // calculate the diff.
> > {  
> >  std::lock_guard Lock(HighlightingsMutex);
> >  FileToHighlightings[File] = std::move(Highlightings);
> > }
> > ```
> I think I changed this to only lock once (and copy instead) at the same time 
> me and @ilya-biryukov were talking about the race condition (?)
It means there's a window where nobody can access the highlightings for this 
file. Which is probably fine, we really do use this only for computing the diff 
and nothing else.

But doing the copy shouldn't matter here either, so leaving as is also looks ok 
from my side.

If you decide to avoid an extra copy, please add comments to 
`FileToHighlightings` (e.g. `only used to compute diffs, must not be used 
outside onHighlightingsReady and didRemove`).
It is declared really close to `FixItsMap` and looks very similar, but the 
latter is used in completely different ways; that could lead to confusion.

Don't remember exact details about race conditions, but we should be good now 
that it's called inside `Publish()`



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408
+}
+
 } // namespace

jvikstrom wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Could you also add a separate test that checks diffs when the number of 
> > > lines is getting smaller (i.e. taking `NLines` into account)
> > I would expect this to be better handled outside `checkDiffedHighlights` to 
> > avoid over-generalizing this function. But up to you.
> That's what this test does: 
> 
> ```
> {
> R"(
> $Class[[A]]
> $Variable[[A]]
> $Class[[A]]
> $Variable[[A]]
>   )",
> R"(
> $Class[[A]]
> $Variable[[A]]
>   )"},
> ```
> 
> But do you mean I should do a testcase like:
> 
> 
> 
> ```
> {
> R"(
> $Class[[A]]
> $Variable[[A]]
> $Class[[A]]
> $Variable[[A]]
>   )",
> R"(
> $Class[[A]]
> $Variable[[A]]
> $Class[[A]]
> $Variable[[A]]
>   )"},
> ```
> And just set NLines = 2 instead when doing the diffing?
> 
Ah, the test actually needs the set of changed lines to be **exactly** the same 
as marked lines.
In that case, all is good, I've just missed this detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[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/ClangdLSPServer.cpp:1125
+Old = std::move(FileToHighlightings[File]);
+FileToHighlightings[File] = Highlightings;
+  }

hokein wrote:
> ilya-biryukov wrote:
> > NIT: avoid copying (from `Highlightings` into the map) under a lock, make 
> > the copy outside the lock instead
> I think we can even avoid copy, since we only use the `Highlightings` for 
> calculating the diff.
> 
> Maybe just 
> 
> ```
> {
> std::lock_guard Lock(HighlightingsMutex);
> Old = std::move(FileToHighlightings[File]);
> }
> // calculate the diff.
> {  
>  std::lock_guard Lock(HighlightingsMutex);
>  FileToHighlightings[File] = std::move(Highlightings);
> }
> ```
I think I changed this to only lock once (and copy instead) at the same time me 
and @ilya-biryukov were talking about the race condition (?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+Old = std::move(FileToHighlightings[File]);
+FileToHighlightings[File] = Highlightings;
+  }

ilya-biryukov wrote:
> NIT: avoid copying (from `Highlightings` into the map) under a lock, make the 
> copy outside the lock instead
I think we can even avoid copy, since we only use the `Highlightings` for 
calculating the diff.

Maybe just 

```
{
std::lock_guard Lock(HighlightingsMutex);
Old = std::move(FileToHighlightings[File]);
}
// calculate the diff.
{  
 std::lock_guard Lock(HighlightingsMutex);
 FileToHighlightings[File] = std::move(Highlightings);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408
+}
+
 } // namespace

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Could you also add a separate test that checks diffs when the number of 
> > lines is getting smaller (i.e. taking `NLines` into account)
> I would expect this to be better handled outside `checkDiffedHighlights` to 
> avoid over-generalizing this function. But up to you.
That's what this test does: 

```
{
R"(
$Class[[A]]
$Variable[[A]]
$Class[[A]]
$Variable[[A]]
  )",
R"(
$Class[[A]]
$Variable[[A]]
  )"},
```

But do you mean I should do a testcase like:



```
{
R"(
$Class[[A]]
$Variable[[A]]
$Class[[A]]
$Variable[[A]]
  )",
R"(
$Class[[A]]
$Variable[[A]]
$Class[[A]]
$Variable[[A]]
  )"},
```
And just set NLines = 2 instead when doing the diffing?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Copy outside lock.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,10 +46,47 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+// Any annotations in OldCode and NewCode are converted into their corresponding
+// HighlightingToken. The tokens are diffed against each other. Any lines where
+// the tokens should diff must be marked with a ^ somewhere on that line in
+// NewCode. If there are diffs that aren't marked with ^ the test fails. The
+// test also fails if there are lines marked with ^ that don't differ.
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  llvm::DenseMap> ExpectedLines;
+  for (const Position  : NewTest.points()) {
+ExpectedLines[Point.line]; // Default initialize to an empty line. Tokens
+   // are inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens))
-   << "Inputs is:\n" << Code;
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -226,8 +261,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -252,21 +288,124 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408
+}
+
 } // namespace

ilya-biryukov wrote:
> Could you also add a separate test that checks diffs when the number of lines 
> is getting smaller (i.e. taking `NLines` into account)
I would expect this to be better handled outside `checkDiffedHighlights` to 
avoid over-generalizing this function. But up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+Old = std::move(FileToHighlightings[File]);
+FileToHighlightings[File] = Highlightings;
+  }

NIT: avoid copying (from `Highlightings` into the map) under a lock, make the 
copy outside the lock instead



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408
+}
+
 } // namespace

Could you also add a separate test that checks diffs when the number of lines 
is getting smaller (i.e. taking `NLines` into account)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 212522.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.

Stopped using expensive getLineNumber function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,10 +46,47 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+// Any annotations in OldCode and NewCode are converted into their corresponding
+// HighlightingToken. The tokens are diffed against each other. Any lines where
+// the tokens should diff must be marked with a ^ somewhere on that line in
+// NewCode. If there are diffs that aren't marked with ^ the test fails. The
+// test also fails if there are lines marked with ^ that don't differ.
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  llvm::DenseMap> ExpectedLines;
+  for (const Position  : NewTest.points()) {
+ExpectedLines[Point.line]; // Default initialize to an empty line. Tokens
+   // are inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens))
-   << "Inputs is:\n" << Code;
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -226,8 +261,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -252,21 +288,124 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:81
+unsigned FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);
+

from the comment of the getLineNumber, this is not cheap to call this method. 
We may use `SM.getBufferData(MainFileID).count('\n')` to count the line numbers.

```
// This requires building and caching a table of line offsets for the
/// MemoryBuffer, so this is not cheap: use only when about to emit a
/// diagnostic.
```



Comment at: clang-tools-extra/clangd/ClangdServer.h:55
   /// Called by ClangdServer when some \p Highlightings for \p File are ready.
-  virtual void
-  onHighlightingsReady(PathRef File,
-   std::vector Highlightings) {}
+  /// \p NLines are the number of lines in the file, needed to make sure that
+  /// any diffing does not add lines beyond EOF.

nit: drop the `needed ...`, this is an interface, don't mention the details of 
subclass (diffs are subclass implementation details).

Maybe name it "NumLines"? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(New.begin(),
+  /*length*/0UL);

ilya-biryukov wrote:
> Could we try to find a better name for this? Having `Line` and `NextLine()` 
> which represent line numbers and `NewLine` which represents highlightings, 
> creates some confusion.
I renamed the `Line` and `NextLine()` instead. Could also rename `NewLine` to 
something like `NewLineHighlightings` but I feel like it almost becomes to 
verbose at that point.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:309
+  // If the New file has fewer lines than the Old file we don't want to send
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits::max() && Line < NLines;

hokein wrote:
> nit: I believe theia is crashing because of this? could we file a bug to 
> theia? I think it would be nice for client to be robust on this case. 
I seem to be misremembering, when I first started testing in theia I think I 
was crashing the client (due to a broken base64 encoding function, will take a 
look and see what was actually happening, can't quite remember)
It actually seems like theia just ignores any out of bounds highlightings.

So this could be removed, it will just sometimes return highlightings that are 
outside of the file and hopefully any future clients would handle that as well. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 212301.
jvikstrom marked 14 inline comments as done.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,10 +46,47 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+// Any annotations in OldCode and NewCode are converted into their corresponding
+// HighlightingToken. The tokens are diffed against each other. Any lines where
+// the tokens should diff must be marked with a ^ somewhere on that line in
+// NewCode. If there are diffs that aren't marked with ^ the test fails. The
+// test also fails if there are lines marked with ^ that don't differ.
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  llvm::DenseMap> ExpectedLines;
+  for (const Position  : NewTest.points()) {
+ExpectedLines[Point.line]; // Default initialize to an empty line. Tokens
+   // are inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens))
-   << "Inputs is:\n" << Code;
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -226,8 +261,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -252,21 +288,124 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

a few more comments from my side.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:309
+  // If the New file has fewer lines than the Old file we don't want to send
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits::max() && Line < NLines;

nit: I believe theia is crashing because of this? could we file a bug to theia? 
I think it would be nice for client to be robust on this case. 



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:70
+// Return a line-by-line diff between two highlightings.
+//  - if the tokens on a line are the same in both hightlightings this line is
+//  omitted.

nit:  add `, ` before `this`



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:73
+//  - if a line exists in New but not in Old the tokens
+//  on this line is emitted., we emit the tokens on this line
+//  - if a line not exists in New but in Old an empty

could you refine this sentence, I can't parse it.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+//  on this line is emitted., we emit the tokens on this line
+//  - if a line not exists in New but in Old an empty
+//  line is emitted (to tell client to clear the previous highlightings on this

and here as well.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef New,
+  ArrayRef Old, int NLines);
 

ilya-biryukov wrote:
> Could you document what `NLines` is? Specifically, whether it's the number of 
> lines for `New` or `Old`.
> 
could we use a more describe name for  `NLines`? maybe `MaxLines`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly final NITs from me, but there is an important one about removing the 
`erase` call on `didOpen`.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);
+  }

Now that the patch landed, this is obsolete. Could you remove?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+FileID MainFileID = SM.getMainFileID();
+unsigned int FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);

NIT: use `unsigned` instead of `unsigned int`



Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+  virtual void onHighlightingsReady(
+  PathRef File, std::vector Highlightings, int NLines) 
{}
 };

NIT: could you document `NLines`



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(New.begin(),
+  /*length*/0UL);

Could we try to find a better name for this? Having `Line` and `NextLine()` 
which represent line numbers and `NewLine` which represents highlightings, 
creates some confusion.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:310
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits::max() && Line < NLines;
+   Line = NextLine()) {

`Line != intmax` is redundant (NLines is <= intmax by definition).
Maybe remove it altogether to simplify the loop condition?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef New,
+  ArrayRef Old, int NLines);
 

Could you document what `NLines` is? Specifically, whether it's the number of 
lines for `New` or `Old`.




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:60
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);

NIT: add documentation on how this should be used
Most importantly, the fact that we need to put `^` on all changed lines should 
be mentioned.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:66
+
+  std::map> ExpectedLines;
+  for (const Position  : NewTest.points()) {

NIT: use `llvm::DenseMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D64475#1602195 , @jvikstrom wrote:

> In D64475#1593481 , @ilya-biryukov 
> wrote:
>
> > The fix for a race condition on remove has landed in rL366577 
> > , this revision would need a small 
> > update after it.
>
>
> Fixed to work with that patch.
>  Should the diffing be moved somewhere else that is not inside the publish 
> function though? That would require moving the state outside the LSP server 
> though and handle the onClose/onOpen events somewhere else though. 
>  Maybe it's ok to keep the diffing in the publish lock?


Yeah, let's keep it in publish for now. Diffing in there does not change 
complexity of the operations.
If it ever turns out that the constant factor is too high, we can figure out 
ways to fight this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added a comment.

In D64475#1593481 , @ilya-biryukov 
wrote:

> The fix for a race condition on remove has landed in rL366577 
> , this revision would need a small update 
> after it.


Fixed to work with that patch.
Should the diffing be moved somewhere else that is not inside the publish 
function though? That would require moving the state outside the LSP server 
though and handle the onClose/onOpen events somewhere else though. 
Maybe it's ok to keep the diffing in the publish lock?




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{

ilya-biryukov wrote:
> @hokein rightfully pointed out that mentioning all changed lines makes the 
> tests unreadable.
> An alternative idea we all came up with is to force people to put `^` on each 
> of the changed lines inside the `NewCode`, i.e.
> 
> ```
> {/*Before*/ R"(
>   $Var[[a]]
>   $Func[[b]]
> "),
>  /*After*/ R"(
>   $Var[[a]]
>  ^$Func[[b]]
> )"} // and no list of lines is needed!
> ```
> 
> Could we do that here?
> 
> One interesting case that we can't test this way to removing lines from the 
> end of the file. But for that particular case, could we just write a separate 
> test case?
We don't want to send diffs that are beyond the end of the file. There is a 
testcase for that as well (the count of newlines in the new code is sent to the 
differ as the number of lines in the file).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 211890.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.

Made tests more readable. Updated to work with rL366577 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,10 +46,42 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens))
-   << "Inputs is:\n" << Code;
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  std::map> ExpectedLines;
+  for (const Position  : NewTest.points()) {
+ExpectedLines[Point.line]; // Default initialize to an empty line. Tokens
+   // are inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -226,8 +256,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -252,21 +283,126 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+  } TestCases[]{
+{
+  R"(
+$Variable[[A]]
+$Class[[B]]
+$Function[[C]]
+  )",
+  R"(
+$Variable[[A]]
+$Class[[D]]
+$Function[[C]]
+  )"},
+{
+  R"(
+$Class[[C]]
+$Field[[F]]
+

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The fix for a race condition on remove has landed in rL366577 
, this revision would need a small update 
after it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{

@hokein rightfully pointed out that mentioning all changed lines makes the 
tests unreadable.
An alternative idea we all came up with is to force people to put `^` on each 
of the changed lines inside the `NewCode`, i.e.

```
{/*Before*/ R"(
  $Var[[a]]
  $Func[[b]]
"),
 /*After*/ R"(
  $Var[[a]]
 ^$Func[[b]]
)"} // and no list of lines is needed!
```

Could we do that here?

One interesting case that we can't test this way to removing lines from the end 
of the file. But for that particular case, could we just write a separate test 
case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+// edit there are stale previous highlightings.
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);

ilya-biryukov wrote:
> jvikstrom wrote:
> > ilya-biryukov wrote:
> > > Should can't we handle this on `didClose` instead?
> > We are removing in didClose but the problem is that there is a race 
> > condition (I think).
> > 
> > If someone does some edits and closes the document right after, the 
> > highlightings for the final edit might finish being generated after the 
> > FileToHighlightings have earsed the highlightings for the file. So next 
> > time when opening the file we will have those final highlightings that were 
> > generated for the last edit in the map. 
> > I don't really know how we could handle this in didClose without having 
> > another map and with an open/closed bit and checking that every time we 
> > generate new highlightings. But we'd still have to set the bit to be open 
> > every didOpen so I don't really see the benefit.
> > 
> > However I've head that ASTWorked is synchronous for a single file, is that 
> > the case for the didClose call as well? Because in that case there is no 
> > race condition.
> You are correct, there is actually a race condition. We worked hard to 
> eliminate it for diagnostics, but highlightings are going through a different 
> code path in `ASTWorker`, not guarded by `DiagsMu` and `ReportDiagnostics`.
> 
> And, unfortunately, I don't think this guard here prevents it in all cases. 
> In particular, there is still a possibility (albeit very low, I guess) that 
> the old highlightings you are trying to remove here are still being computed. 
> If they are reported **after** this `erase()` runs, we will end up with 
> inconsistent highlightings.
> 
> Ideally, we would guard the diagnostics callback with the same mutex, but 
> given our current layering it seems like a hard problem, will need to think 
> what's the simplest way to fix this.
The race condition of highlighting sounds bad (especially when a user opens a 
large file and closes it immediately, then the highlighting is finished and we 
emit it to the client). No need to fix it in this patch, just add a FIXME.


Can we use the same mechanism for Diagnostic to guard the highlighting here? or 
use the `DiagsMu` and `ReportDiagnostics` to guard the `callback.onMainAST()` 
as well (which is probably not a good idea)...



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {

jvikstrom wrote:
> ilya-biryukov wrote:
> > Can we test this in a more direct manner by specifying:
> > 1. annotated input for old highlightings,
> > 2. annotated input for new highlightings,
> > 3. the expected diff?
> > 
> > The resulting tests don't have to be real C++ then, allowing to express 
> > what we're testing in a more direct manner.
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*Diff*/ "{{/*line */0, 
> > "$Class[[a]]"}}
> > ```
> > 
> > It also seems that the contents of the lines could even be checked 
> > automatically (they should be equal to the corresponding line from 
> > `/*New*/`, right?), that leaves us with even simpler inputs:
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*DiffLines*/ "{0}}
> > ```
> That's a great idea on how to write these tests.
hmm, I have a different opinion here (I'd prefer the previous one), let's chat.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:292
+  /*length*/0UL);
+  auto NewEnd = NewHighlightings.end();
+  auto OldEnd = OldHighlightings.end();

ilya-biryukov wrote:
> NIT: maybe just inline `NewEnd` and `OldEnd` to save a few lines?
Wouldn't that violate code style: 
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
 ?
Or maybe that doesn't matter? (I have no preference either way, just picked 
this way as I could remeber reading from the styleguide)



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {

ilya-biryukov wrote:
> Can we test this in a more direct manner by specifying:
> 1. annotated input for old highlightings,
> 2. annotated input for new highlightings,
> 3. the expected diff?
> 
> The resulting tests don't have to be real C++ then, allowing to express what 
> we're testing in a more direct manner.
> ```
> {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*Diff*/ "{{/*line */0, 
> "$Class[[a]]"}}
> ```
> 
> It also seems that the contents of the lines could even be checked 
> automatically (they should be equal to the corresponding line from `/*New*/`, 
> right?), that leaves us with even simpler inputs:
> ```
> {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*DiffLines*/ "{0}}
> ```
That's a great idea on how to write these tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Removed one lock from onSemanticHighlighting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +46,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode,
+   llvm::ArrayRef DiffedLines) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  std::map> ExpectedLines;
+  for (int Line : DiffedLines) {
+ExpectedLines[Line]; // Default initialize to an empty line. Tokens are
+ // inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -225,8 +257,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -251,21 +284,135 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{
+{
+  R"cpp(
+$Variable[[A]]
+$Class[[B]]
+$Function[[C]]
+  )cpp",
+  R"cpp(
+$Variable[[A]]
+$Class[[D]]
+$Function[[C]]
+  )cpp",
+  {}},
+{
+  R"cpp(
+$Class[[C]]
+$Field[[F]]
+$Variable[[V]]
+$Class[[C]] 

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Fixed test case that was actually (kinda) broken even though it didn't fail.

(The last two edits in semantic-highlighting test. They removed the `= 2` part 
and did not insert an extra `;` which produced invalid code. It was still fine 
because the highlightings that were supposed to be generated were still 
generated. This just makes the code not be invalid)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +46,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode,
+   llvm::ArrayRef DiffedLines) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  std::map> ExpectedLines;
+  for (int Line : DiffedLines) {
+ExpectedLines[Line]; // Default initialize to an empty line. Tokens are
+ // inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -225,8 +257,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -251,21 +284,135 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{
+{

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210600.
jvikstrom marked 7 inline comments as done.
jvikstrom added a comment.

Address comments and fixed bug where we'd send diffs outside of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +46,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode,
+   llvm::ArrayRef DiffedLines) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  std::map> ExpectedLines;
+  for (int Line : DiffedLines) {
+ExpectedLines[Line]; // Default initialize to an empty line. Tokens are
+ // inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -225,8 +257,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -251,21 +284,137 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{
+{
+  R"cpp(
+$Variable[[A]]
+$Class[[B]]
+$Function[[C]]
+  )cpp",
+  R"cpp(
+$Variable[[A]]
+$Class[[D]]
+$Function[[C]]
+  )cpp",
+  {}},
+{
+  R"cpp(
+

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Remove braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,7 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +46,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::vector ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode,
+   llvm::ArrayRef DiffedLines) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldTokens = getExpectedTokens(OldTest);
+  std::vector NewTokens = getExpectedTokens(NewTest);
+
+  std::map> ExpectedLines;
+  for (int Line : DiffedLines) {
+ExpectedLines[Line]; // Default initialize to an empty line. Tokens are
+ // inserted on these lines later.
+  }
+  std::vector ExpectedLinePairHighlighting;
+  for (const HighlightingToken  : NewTokens) {
+auto It = ExpectedLines.find(Token.R.start.line);
+if (It != ExpectedLines.end())
+  It->second.push_back(Token);
+  }
+  for (auto  : ExpectedLines)
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+  std::vector ActualDiffed =
+  diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -225,8 +257,9 @@
 std::atomic Count = {0};
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
-void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+void onHighlightingsReady(PathRef File,
+  std::vector Highlightings,
+  int NLines) override {
   ++Count;
 }
   };
@@ -251,21 +284,135 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+std::vector DiffedLines;
+  } TestCases[]{
+{
+  R"cpp(
+$Variable[[A]]
+$Class[[B]]
+$Function[[C]]
+  )cpp",
+  R"cpp(
+$Variable[[A]]
+$Class[[D]]
+$Function[[C]]
+  )cpp",
+  {}},
+{
+  R"cpp(
+$Class[[C]]
+$Field[[F]]
+$Variable[[V]]
+$Class[[C]] $Variable[[V]] $Field[[F]]
+  

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+// edit there are stale previous highlightings.
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);

jvikstrom wrote:
> ilya-biryukov wrote:
> > Should can't we handle this on `didClose` instead?
> We are removing in didClose but the problem is that there is a race condition 
> (I think).
> 
> If someone does some edits and closes the document right after, the 
> highlightings for the final edit might finish being generated after the 
> FileToHighlightings have earsed the highlightings for the file. So next time 
> when opening the file we will have those final highlightings that were 
> generated for the last edit in the map. 
> I don't really know how we could handle this in didClose without having 
> another map and with an open/closed bit and checking that every time we 
> generate new highlightings. But we'd still have to set the bit to be open 
> every didOpen so I don't really see the benefit.
> 
> However I've head that ASTWorked is synchronous for a single file, is that 
> the case for the didClose call as well? Because in that case there is no race 
> condition.
You are correct, there is actually a race condition. We worked hard to 
eliminate it for diagnostics, but highlightings are going through a different 
code path in `ASTWorker`, not guarded by `DiagsMu` and `ReportDiagnostics`.

And, unfortunately, I don't think this guard here prevents it in all cases. In 
particular, there is still a possibility (albeit very low, I guess) that the 
old highlightings you are trying to remove here are still being computed. If 
they are reported **after** this `erase()` runs, we will end up with 
inconsistent highlightings.

Ideally, we would guard the diagnostics callback with the same mutex, but given 
our current layering it seems like a hard problem, will need to think what's 
the simplest way to fix this.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector
+diffHighlightings(ArrayRef NewHighlightings,
+  ArrayRef OldHighlightings);

jvikstrom wrote:
> ilya-biryukov wrote:
> > Are we locked into the line-based diff implementation?
> > It works nicely while editing inside the same line, but seem to do a bad 
> > job on a common case of inserting/removing lines.
> > 
> > Does the protocol have a way to communicate this cleanly?
> We are looked into some kind of line based diff implementation as the 
> protocol sends token line by line. 
> There should be away of solving the inserting/removing lines, but I'll have a 
> look at that in a follow up. 
> Theia seems to do a good job of moving tokens to where they should be 
> automatically when inserting a new line though. I want to be able to see how 
> vscode handles it first as well though before.
It seems there no compact way to send interesting diffs then, although the 
clients can do a reasonably good job of moving the older version of 
highlightings on changes until the server sends them a new version.

The diff-based approach only seems to be helping with changes on a single line.

Which is ok, thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {

Can we test this in a more direct manner by specifying:
1. annotated input for old highlightings,
2. annotated input for new highlightings,
3. the expected diff?

The resulting tests don't have to be real C++ then, allowing to express what 
we're testing in a more direct manner.
```
{/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*Diff*/ "{{/*line */0, 
"$Class[[a]]"}}
```

It also seems that the contents of the lines could even be checked 
automatically (they should be equal to the corresponding line from `/*New*/`, 
right?), that leaves us with even simpler inputs:
```
{/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*DiffLines*/ "{0}}
```



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:391
+  for (const auto  : TestCases) {
+checkDiffedHighlights(Test.OldCode, Test.NewCode);
+  }

NIT: remove braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210567.
jvikstrom edited the summary of this revision.
jvikstrom added a comment.

Added additional note to FIXME on how to solve and also move(ing) Old.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,12 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
+std::vector getActualTokens(Annotations ) {
   auto AST = TestTU::withCode(Test.code()).build();
+  return getSemanticHighlightings(AST);
+}
+
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +51,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+std::vector getExpectedEmptyLines(Annotations ) {
+  auto EmptyRanges = Test.ranges("Empty");
+  std::vector EmptyLines;
+  EmptyLines.reserve(EmptyRanges.size());
+  for (const auto  : EmptyRanges)
+EmptyLines.push_back(EmptyRange.start.line);
+  return EmptyLines;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  EXPECT_THAT(getActualTokens(Test),
+  getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : getExpectedEmptyLines(NewTest))
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : getExpectedTokens(NewTest))
+ExpectedLines[Token.R.start.line].push_back(Token);
+  for (auto  : ExpectedLines) {
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  std::vector ActualDiffed =
+  diffHighlightings(getActualTokens(NewTest), getActualTokens(OldTest));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -251,21 +288,110 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+  } TestCases[]{{
+  R"cpp(
+int A
+double B;
+struct C {};
+  )cpp",
+  R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"},
+{
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta {};
+int A = 121;
+Alpha Var;
+  )cpp",
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta   {}; // Some comment
+$Variable[[intA]] = 121;
+$Class[[Beta]] $Variable[[Var]];
+  )cpp"},
+{
+  R"cpp(
+int A = 121; int B;
+  )cpp",
+  R"cpp(
+$Variable[[intA]] = 121; int $Variable[[B]];
+  )cpp"},
+{
+  R"cpp(
+int

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom marked 6 inline comments as done.
jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+// edit there are stale previous highlightings.
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);

ilya-biryukov wrote:
> Should can't we handle this on `didClose` instead?
We are removing in didClose but the problem is that there is a race condition 
(I think).

If someone does some edits and closes the document right after, the 
highlightings for the final edit might finish being generated after the 
FileToHighlightings have earsed the highlightings for the file. So next time 
when opening the file we will have those final highlightings that were 
generated for the last edit in the map. 
I don't really know how we could handle this in didClose without having another 
map and with an open/closed bit and checking that every time we generate new 
highlightings. But we'd still have to set the bit to be open every didOpen so I 
don't really see the benefit.

However I've head that ASTWorked is synchronous for a single file, is that the 
case for the didClose call as well? Because in that case there is no race 
condition.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:276
+  ArrayRef OldHighlightings) {
+  // FIXME: There's an edge case when tokens span multiple lines. If the first
+  // token on the line started on a line above the current one and the rest of

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Do you have any ideas on how we can fix this?
> > 
> > I would simply split those tokens into multiple tokens of the same kind at 
> > newlines boundaries, but maybe there are other ways to handle this.
> > 
> > In any case, could you put a suggested way to fix this (in case someone 
> > will want to pick it up, they'll have a starting point)
> NIT: add assertions that highlightings are sorted.
Yeah, I would split the tokens into multiple lines as well. Or enforce that a 
token is single line and handle it in addToken in the collector. (i.e. change 
the HighlightingToken struct)

It's a bit annoying to solve because we'd have to get the line lengths of every 
line that the multiline length token covers when doing that.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector
+diffHighlightings(ArrayRef NewHighlightings,
+  ArrayRef OldHighlightings);

ilya-biryukov wrote:
> Are we locked into the line-based diff implementation?
> It works nicely while editing inside the same line, but seem to do a bad job 
> on a common case of inserting/removing lines.
> 
> Does the protocol have a way to communicate this cleanly?
We are looked into some kind of line based diff implementation as the protocol 
sends token line by line. 
There should be away of solving the inserting/removing lines, but I'll have a 
look at that in a follow up. 
Theia seems to do a good job of moving tokens to where they should be 
automatically when inserting a new line though. I want to be able to see how 
vscode handles it first as well though before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:274
+std::vector
+diffHighlightings(ArrayRef NewHighlightings,
+  ArrayRef OldHighlightings) {

NIT: maybe rename to `New` and `Old`, the suffix of the name could be easily 
inferred from the variable type (clangd has hover/go-to-definition to find the 
type quickly, the code is rather small so its should always be visible on the 
screen too).



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:276
+  ArrayRef OldHighlightings) {
+  // FIXME: There's an edge case when tokens span multiple lines. If the first
+  // token on the line started on a line above the current one and the rest of

ilya-biryukov wrote:
> Do you have any ideas on how we can fix this?
> 
> I would simply split those tokens into multiple tokens of the same kind at 
> newlines boundaries, but maybe there are other ways to handle this.
> 
> In any case, could you put a suggested way to fix this (in case someone will 
> want to pick it up, they'll have a starting point)
NIT: add assertions that highlightings are sorted.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:286
+  // incorrectly removed.
+  std::vector LineTokens;
+  // ArrayRefs to the current line in the highlights.

NIT: maybe mention `diff` in the name somehow. I was confused at first, as I 
thought it's all highlightings grouped by line, not the diff.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:292
+  /*length*/0UL);
+  auto NewEnd = NewHighlightings.end();
+  auto OldEnd = OldHighlightings.end();

NIT: maybe just inline `NewEnd` and `OldEnd` to save a few lines?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  auto OldEnd = OldHighlightings.end();
+  for (int Line = 0; NewLine.end() < NewEnd || OldLine.end() < OldEnd; ++Line) 
{
+NewLine = takeLine(NewHighlightings, NewLine.end(), Line);

Could we skip directly to the next interesting line?
The simplest way to do this I could come up with is this:
```
auto NextLine = [&]() {
  int NextNew = NewLine.end() != NewHighlightings.end() ? 
NewLine.end()->start.line : numeric_limits::max();
  int NextOld = ...;
  return std::min(NextNew, NextOld);
}

for (int Line = 0; ...; Line = NextLine()) {
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

A few suggestions from me, I haven't looked into the diffing algorithm and the 
tests yet.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+// edit there are stale previous highlightings.
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);

Should can't we handle this on `didClose` instead?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1115
+std::lock_guard Lock(HighlightingsMutex);
+Old = FileToHighlightings[File];
+  }

hokein wrote:
> use std::move() to save a copy here. we'd drop the old highlighting anyway 
> (replace it with new highlightings).
NIT: maybe std::move() into Old?

If we had exceptions, that would be complicated, but we don't have them



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1124
+  {
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings[File] = std::move(Highlightings);

To follow the same pattern as diagnostics, could we store before calling 
`publish...`?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:276
+  ArrayRef OldHighlightings) {
+  // FIXME: There's an edge case when tokens span multiple lines. If the first
+  // token on the line started on a line above the current one and the rest of

Do you have any ideas on how we can fix this?

I would simply split those tokens into multiple tokens of the same kind at 
newlines boundaries, but maybe there are other ways to handle this.

In any case, could you put a suggested way to fix this (in case someone will 
want to pick it up, they'll have a starting point)



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector
+diffHighlightings(ArrayRef NewHighlightings,
+  ArrayRef OldHighlightings);

Are we locked into the line-based diff implementation?
It works nicely while editing inside the same line, but seem to do a bad job on 
a common case of inserting/removing lines.

Does the protocol have a way to communicate this cleanly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

one missing point -- could you please update the patch description?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[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 from my side.

@ilya-biryukov will be nice if you could take a second look on the patch. We 
plan to land it before the release cut today.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1115
+std::lock_guard Lock(HighlightingsMutex);
+Old = FileToHighlightings[File];
+  }

use std::move() to save a copy here. we'd drop the old highlighting anyway 
(replace it with new highlightings).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210527.
jvikstrom marked 9 inline comments as done.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,12 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
+std::vector getActualTokens(Annotations ) {
   auto AST = TestTU::withCode(Test.code()).build();
+  return getSemanticHighlightings(AST);
+}
+
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -48,9 +51,43 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  llvm::sort(ExpectedTokens);
+  return ExpectedTokens;
+}
 
-  auto ActualTokens = getSemanticHighlightings(AST);
-  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+std::vector getExpectedEmptyLines(Annotations ) {
+  auto EmptyRanges = Test.ranges("Empty");
+  std::vector EmptyLines;
+  EmptyLines.reserve(EmptyRanges.size());
+  for (const auto  : EmptyRanges)
+EmptyLines.push_back(EmptyRange.start.line);
+  return EmptyLines;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  EXPECT_THAT(getActualTokens(Test),
+  getExpectedTokens(Test));
+}
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : getExpectedEmptyLines(NewTest))
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : getExpectedTokens(NewTest))
+ExpectedLines[Token.R.start.line].push_back(Token);
+  for (auto  : ExpectedLines) {
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  std::vector ActualDiffed =
+  diffHighlightings(getActualTokens(NewTest), getActualTokens(OldTest));
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -251,21 +288,110 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
+llvm::StringRef OldCode;
+llvm::StringRef NewCode;
+  } TestCases[]{{
+  R"cpp(
+int A
+double B;
+struct C {};
+  )cpp",
+  R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"},
+{
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta {};
+int A = 121;
+Alpha Var;
+  )cpp",
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta   {}; // Some comment
+$Variable[[intA]] = 121;
+$Class[[Beta]] $Variable[[Var]];
+  )cpp"},
+{
+  R"cpp(
+int A = 121; int B;
+  )cpp",
+  R"cpp(
+$Variable[[intA]] = 121; int $Variable[[B]];
+  )cpp"},
+{
+  R"cpp(
+intA;
+  )cpp",
+  R"cpp(
+struct 

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:119
   void addToken(SourceLocation Loc, const NamedDecl *D) {
+if (D->isInvalidDecl())
+  return;

This means if we won't emit most of symbols if the AST is ill-formed? I'm not 
sure whether we should include it in this patch, it seems out of scope. could 
we leave it out in this patch?





Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:68
+
+/// Removes every line where \c Highlightings is the same as \c
+/// PrevHighlightings. If \c PrevHighlightings has lines that does not exist

The comment seems a bit out of date (as the code is updated during review), and 
it should mention the diff strategy of this function.

```
// Return a line-by-line diff between two highlightings.
//  - if the tokens on a line are the same in both hightlightings, we omit this 
line
//  - if a line exists in NewHighligtings but not in OldHighligtings, we emit 
the tokens on this line
//  - if a line not exists in NewHighligtings but in OldHighligtings, we emit 
an empty line (to tell client not highlighting this line)
``` 



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:53
   }
+  return ExpectedTokens;
+}

nit: let's sort the tokens here to make the result deterministic.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:59
+  std::vector EmptyLines(EmptyRanges.size());
+  for (unsigned I = 0, End = EmptyRanges.size(); I < End; ++I)
+EmptyLines[I] = EmptyRanges[I].start.line;

nit: you could use for-range loop to simplify the code.

```
for (const auto& EmptyRange : EmptyRanges)
   EmptyLines.push_back(EmptyRange.start.line);
```



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:75
+  std::vector OldActualTokens = getActualTokens(OldTest);
+  std::vector NewActualTokens = getActualTokens(NewTest);
+  std::vector ExpectedTokens = getExpectedTokens(NewTest);

nit: since `OldActualTokens` and `NewActualTokens` are used only once in 
`diffHighlightings`, I'd inline them there (instead of creating two new 
variables).



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:88
+  for (auto  : ExpectedLines) {
+llvm::sort(LineTokens.second);
+ExpectedLinePairHighlighting.push_back(

if we do sort in `getExpectedTokens`, we don't need this here.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:274
+  // The first entry is the old code. The second entry is the new code.
+  std::vector> TestCases{{
+  R"cpp(

nit: Use an explicit struct, then you wouldn't need a comment.

```
struct {
   llvm::StringRef OldCode;
   llvm::StringRef NewCode;
} TestCases[] = {
   ...
}
```



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:275
+  std::vector> TestCases{{
+  R"cpp(
+int A

could we add one more case?

```
// Before
int a;
int b;
int c;

// After
int a;
int $Variable[[new_b]];
int c;
```



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:327
+
+  for (auto Test : TestCases) {
+checkDiffedHighlights(Test.first, Test.second);

nit: `const auto&`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210297.
jvikstrom marked 6 inline comments as done.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,9 +29,12 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
-  Annotations Test(Code);
+std::vector getActualTokens(Annotations ) {
   auto AST = TestTU::withCode(Test.code()).build();
+  return getSemanticHighlightings(AST);
+}
+
+std::vector getExpectedTokens(Annotations ) {
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
@@ -47,11 +50,50 @@
 Test.ranges(KindString.second), KindString.first);
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
+  return ExpectedTokens;
+}
 
-  auto ActualTokens = getSemanticHighlightings(AST);
+std::vector getExpectedEmptyLines(Annotations ) {
+  auto EmptyRanges = Test.ranges("Empty");
+  std::vector EmptyLines(EmptyRanges.size());
+  for (unsigned I = 0, End = EmptyRanges.size(); I < End; ++I)
+EmptyLines[I] = EmptyRanges[I].start.line;
+  return EmptyLines;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  std::vector ActualTokens = getActualTokens(Test);
+  std::vector ExpectedTokens = getExpectedTokens(Test);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector OldActualTokens = getActualTokens(OldTest);
+  std::vector NewActualTokens = getActualTokens(NewTest);
+  std::vector ExpectedTokens = getExpectedTokens(NewTest);
+  std::vector EmptyLines = getExpectedEmptyLines(NewTest);
+  std::vector ActualDiffed =
+  diffHighlightings(NewActualTokens, OldActualTokens);
+
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto  : ExpectedLines) {
+llvm::sort(LineTokens.second);
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -211,21 +253,82 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  // The first entry is the old code. The second entry is the new code.
+  std::vector> TestCases{{
+  R"cpp(
+int A
+double B;
+struct C {};
+  )cpp",
+  R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"},
+{
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta {};
+int A = 121;
+Alpha Var;
+  )cpp",
+  R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta   {}; // Some comment
+$Empty[[]]intA = 121;
+

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:275
+bool operator<(const HighlightingToken , const HighlightingToken ) {
+  return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);;
+}

nit: remove the redundant `;`.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:35
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);

could we split it into three functions?

- getExpectedHighlightings
- getActualHighlightings
- getExpectedEmptyLines





Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:70
 
+void checkDiffedHighlights(const char *OrgCode, const char *NewCode) {
+  std::vector CompleteTokens1;

nit: OrgCode => OldCode, use llvm::StringRef



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:98
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+llvm::sort(ActualDiffed[I].Tokens);
+

I think the tokens are also sorted, as we pass two sorted lists of 
highlightings to `diffHighlightings`.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:282
+  std::vector> TestCases{{
+   R"cpp(
+  int $Variable[[A]]

nit: the code indentation is strange



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:283
+   R"cpp(
+  int $Variable[[A]]
+  double $Variable[[B]];

The annotations in the OldCode are not used -- you only use the actual 
highlightings in the `checkDiffedHighlights`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:281
+  )cpp"}},
+{{5},
+ {

hokein wrote:
> so the empty lines are stored separately, which is not easy to figure out 
> from the testing code snippet. I think we should make the empty lines more 
> obvious in the code snippet (maybe use an `Empty` annotations?) , and 
> explicitly verify the empty lines.
> 
> What do you think refining the test like below, just annotate the diff 
> tokens? I find it is easy to spot the diff tokens.
> 
> ```
> struct Testcase {
>Code Before;
>Code After;
> };
> 
> std::vector cases = {
>{ 
>R"cpp(
>  int a;
>  int b; 
>  int c;
>   )cpp",
>   R"cpp(
> int a;
> $Empty[[]] // int b
> int $Variable[[C]];
>   )cpp"
>   }
> }
> 
> oldHighlightings = getSemanticHighlightings(OldAST);
> newHighlightings = getSemanticHighlightings(NewAST);
> diffHighlightings = diff...;
> // verify the diffHighlightings has the expected empty lines ("Empty" 
> annotations).
> // verify the diffHighlightings has the expected highlightings (the regular 
> annotations);
> ```
> 
Moved everything into the checkDiffedHighlights as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210282.
jvikstrom marked 4 inline comments as done.
jvikstrom added a comment.

Rewrote test to be more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,7 +29,10 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+// Returns ActualTokens, ExpectedTokens and the lines that should be empty.
+std::tuple, std::vector,
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
@@ -48,10 +51,56 @@
 ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
 
+  auto EmptyRanges = Test.ranges("Empty");
+  std::vector EmptyLines(EmptyRanges.size());
+  for (unsigned I = 0, End = EmptyRanges.size(); I < End; ++I)
+EmptyLines[I] = EmptyRanges[I].start.line;
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {ActualTokens, ExpectedTokens, EmptyLines};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(ActualTokens, ExpectedTokens, std::ignore) =
+  getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(const char *OrgCode, const char *NewCode) {
+  std::vector CompleteTokens1;
+  std::tie(CompleteTokens1, std::ignore, std::ignore) =
+  getHighlightingsAnnotated(OrgCode);
+  std::vector CompleteTokens2;
+  std::vector ExpectedTokens;
+  std::vector EmptyLines;
+  std::tie(CompleteTokens2, ExpectedTokens, EmptyLines) =
+  getHighlightingsAnnotated(NewCode);
+
+  std::vector ActualDiffed =
+  diffHighlightings(CompleteTokens2, CompleteTokens1);
+
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto  : ExpectedLines) {
+llvm::sort(LineTokens.second);
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  // The UnorderedElementsAreArray only checks that the top level vector
+  // is unordered. The vectors in the pair must be in the correct order.
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+llvm::sort(ActualDiffed[I].Tokens);
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -211,21 +260,82 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  // The first entry is the old code. The second entry is the new code.
+  std::vector> TestCases{{
+   R"cpp(
+  int $Variable[[A]]
+  double $Variable[[B]];
+  struct $Class[[C]] {};
+)cpp",
+   R"cpp(
+  int A;
+  double B;
+  struct C {};
+)cpp"},
+   {
+   

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(Highlightings.data(),
+  Highlightings.data()),

jvikstrom wrote:
> hokein wrote:
> > IIUC, we are initializing an empty ArrayRef, if so, how about using 
> > `NewLine(Highlightings.data(), /*length*/0)`? 
> > `NewLine(Highlightings.data(), Highlightings.data())` is a bit weird, it 
> > took me a while to understand the purpose.
> I couldn't do that because the `ArrayRef(const T *data, size_t length)` and 
> `ArrayRef(const T *begin, const T *end)` were ambiguous. Added a cast to cast 
> 0 to a size_t which solved it.
you could also do it like `NewLine(Highlightings.data(), /*length*/0UL);`, 
which saves you a `static_cast<>`.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:281
+  )cpp"}},
+{{5},
+ {

so the empty lines are stored separately, which is not easy to figure out from 
the testing code snippet. I think we should make the empty lines more obvious 
in the code snippet (maybe use an `Empty` annotations?) , and explicitly verify 
the empty lines.

What do you think refining the test like below, just annotate the diff tokens? 
I find it is easy to spot the diff tokens.

```
struct Testcase {
   Code Before;
   Code After;
};

std::vector cases = {
   { 
   R"cpp(
 int a;
 int b; 
 int c;
  )cpp",
  R"cpp(
int a;
$Empty[[]] // int b
int $Variable[[C]];
  )cpp"
  }
}

oldHighlightings = getSemanticHighlightings(OldAST);
newHighlightings = getSemanticHighlightings(NewAST);
diffHighlightings = diff...;
// verify the diffHighlightings has the expected empty lines ("Empty" 
annotations).
// verify the diffHighlightings has the expected highlightings (the regular 
annotations);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 210253.
jvikstrom marked 15 inline comments as done.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,7 +29,8 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+std::pair, std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
@@ -49,9 +50,40 @@
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {ActualTokens, ExpectedTokens};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(ActualTokens, ExpectedTokens) = getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(const std::vector ,
+   const std::vector ,
+   std::vector ) {
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto  : ExpectedLines) {
+llvm::sort(LineTokens.second);
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  // The UnorderedElementsAreArray only checks that the top level vector
+  // is unordered. The vectors in the pair must be in the correct order.
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+llvm::sort(ActualDiffed[I].Tokens);
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -211,21 +243,102 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  // Each entry is a test case. An entry consists of two things. The first entry
+  // in the pair is every empty line that should be returned by the diffing. The
+  // second entry are the two code samples that should be diffed (first entry
+  // are the old highlightings and the second entry are the new highlightings.)
+  std::vector<
+  std::pair, std::pair>>
+  TestCases{{{},
+ {
+ R"cpp(
+int $Variable[[A]]
+double $Variable[[B]];
+struct $Class[[C]] {};
+  )cpp",
+ R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"}},
+{{5},
+ {
+ R"cpp(
+  struct $Class[[Alpha]] {
+double SomeVariable = 9483.301;
+  };
+  struct $Class[[Beta]] {};
+  int $Variable[[A]] = 121;
+  $Class[[Alpha]] $Variable[[Var]];
+  )cpp",
+ R"cpp(
+  struct Alpha {
+double SomeVariable = 9483.301;
+  };
+  struct Beta   {}; // Some comment
+  intA = 121;
+  $Class[[Beta]] $Variable[[Var]];
+  )cpp"}},
+{{},
+ {
+ R"cpp(
+  int 

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1110
 PathRef File, std::vector Highlightings) {
+  llvm::ArrayRef Prev;
+  {

hokein wrote:
> this seems unsafe, we get a reference of the map value, we might access it 
> without the mutex being guarded. 
> 
> ```
> std::vector Old;
> {
> std::lock_guard Lock(HighlightingsMutex);
> Old = std::move(FileToHighlightings[File]);
> }
> ```
Aren't the parsing callbacks thread safe in the same file? I think @sammccall 
said so above. 

Although actually, the map might reallocate and move around the memory if 
things are inserted/deleted invalidating the reference maybe (I have no idea of 
how StringMap memory allocation works).

So I guess it doesn't hurt to be safe and copy it.   



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(Highlightings.data(),
+  Highlightings.data()),

hokein wrote:
> IIUC, we are initializing an empty ArrayRef, if so, how about using 
> `NewLine(Highlightings.data(), /*length*/0)`? `NewLine(Highlightings.data(), 
> Highlightings.data())` is a bit weird, it took me a while to understand the 
> purpose.
I couldn't do that because the `ArrayRef(const T *data, size_t length)` and 
`ArrayRef(const T *begin, const T *end)` were ambiguous. Added a cast to cast 0 
to a size_t which solved it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the code looks clearer now!




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:609
 FixItsMap.erase(File);
+std::lock_guard HLock(HighlightingsMutex);
+FileToPrevHighlightings.erase(File);

nit: I would create a new `{}` block merely for the hightlightings.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1110
 PathRef File, std::vector Highlightings) {
+  llvm::ArrayRef Prev;
+  {

this seems unsafe, we get a reference of the map value, we might access it 
without the mutex being guarded. 

```
std::vector Old;
{
std::lock_guard Lock(HighlightingsMutex);
Old = std::move(FileToHighlightings[File]);
}
```



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1124
+std::lock_guard Lock(HighlightingsMutex);
+FileToPrevHighlightings[File] = Highlightings;
+  }

`FileToPrevHighlightings[File] = std::move(Highlightings);`



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:140
+  std::mutex HighlightingsMutex;
+  llvm::StringMap> FileToPrevHighlightings;
 

nit: I'd drop `Prev`.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:229
+// Get the highlights for \c Line where the first entry for \c Line is
+// immediately preceded by \c OldLine
+ArrayRef takeLine(ArrayRef AllTokens,

nit: I'm not sure I understand the comment.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:233
+ int Line) {
+  return ArrayRef(OldLine.end(), AllTokens.end())
+  .take_while([Line](const HighlightingToken ) {

nit: this implies that OldLine and AllTokens must ref to the same container, 
could you refine the `OldLine` as a start `Iterator`? 



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(Highlightings.data(),
+  Highlightings.data()),

IIUC, we are initializing an empty ArrayRef, if so, how about using 
`NewLine(Highlightings.data(), /*length*/0)`? `NewLine(Highlightings.data(), 
Highlightings.data())` is a bit weird, it took me a while to understand the 
purpose.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:257
+  Highlightings.data()),
+  OldLine = {PrevHighlightings.data(), PrevHighlightings.data()};
+  // ArrayRefs to the new and old highlighting vectors.

nit: split it into a new statement.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:259
+  // ArrayRefs to the new and old highlighting vectors.
+  ArrayRef Current = {Highlightings},
+  Prev = {PrevHighlightings};

we don't change Current and Prev below, how about re-using `Highlightings` and 
`PrevHighlightings`?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:71
+/// in \c Highlightings an empty line is added. Returns the resulting
+/// HighlightingTokens grouped by their line number. Assumes the highlightings
+/// are sorted by the tokens' ranges.

I believe `Assumes the highlightings are sorted by the tokens' ranges.` is a 
requirement for this function?

If so, mark it more explicitly in the comment, like 

```
///
/// REQUIRED: OldHighlightings and NewHighlightings are sorted.
```



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+std::vector
+diffHighlightings(ArrayRef Highlightings,
+  ArrayRef PrevHighlightings);

nit: I'd use name the parameters symmetrically, `NewHighlightings` and 
`OldHighlightings`.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:32
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple,
+   std::vector>

we don't need `ParsedAST`  now I think.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:265
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector<
+  std::pair, std::pair>>

this is a complicated structure, could you add some comments describing the 
test strategy here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Moved highlighting state to LSP layer. Removed class holding state. Addressed 
comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,7 +29,9 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple,
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
@@ -49,9 +51,41 @@
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {std::move(AST), ActualTokens, ExpectedTokens};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(std::ignore, ActualTokens, ExpectedTokens) =
+  getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(const std::vector ,
+   const std::vector ,
+   std::vector ) {
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto  : ExpectedLines) {
+llvm::sort(LineTokens.second);
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  // The UnorderedElementsAreArray only checks that the top level vector
+  // is unordered. The vectors in the pair must be in the correct order.
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+llvm::sort(ActualDiffed[I].Tokens);
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -211,21 +245,103 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector<
+  std::pair, std::pair>>
+  TestCases{{{},
+ {
+ R"cpp(
+int $Variable[[A]]
+double $Variable[[B]];
+struct $Class[[C]] {};
+  )cpp",
+ R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"}},
+{{5},
+ {
+ R"cpp(
+  struct $Class[[Alpha]] {
+double SomeVariable = 9483.301;
+  };
+  struct $Class[[Beta]] {};
+  int $Variable[[A]] = 121;
+  $Class[[Alpha]] $Variable[[Var]];
+  )cpp",
+ R"cpp(
+  struct Alpha {
+double SomeVariable = 9483.301;
+  };
+  struct Beta   {}; // Some comment
+  intA = 121;
+  $Class[[Beta]] $Variable[[Var]];
+  )cpp"}},
+{{},
+ {
+ R"cpp(
+  int $Variable[[A]] = 121; int $Variable[[B]];
+)cpp",
+ R"cpp(
+  intA = 121; int $Variable[[B]];
+)cpp"}},
+{{},
+ {
+ R"cpp(
+  int$Variable[[A]];
+  

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

As discussed offline, it's a little scary that the highlight state for diffing 
lives in a separate map from TUScheduler's workers, but it's hard to fix 
without introducing lots of abstractions or layering violations.

It does seem like a good idea to move the diffing up to ClangdLSPServer, as 
it's fairly bound to protocol details. It needs to be reset in didClose (and 
maybe didOpen too, for safety - I think there's a race when a document is 
closed, then tokens are delivered...).

I'd tend to put the map/mutex directly in ClangdLSPServer rather than 
encapsulating it in the Differ object (again, because it's a protocol/lifetime 
detail rather than diffing logic itself) but up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

based on the offline discussion, now I understand the patch better (thanks).

some comments on the interface.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60
+  PathRef File,
+  std::vector>> 
Highlightings)
+  override;

how about creating a new structure `LineHighlightingTokens` to represent the 
line-based tokens?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:82
   bool SemanticHighlighting;
+  HighlightingDiffer Differ;
 };

If we make it as a pointer, the `bool SemanticHighlighting` is not needed (just 
follow the what the other field `FIndex` does).



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  const FileEntry *CurrentFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  std::string CurrentPath = CurrentFileEntry->tryGetRealPathName().str() +
+CurrentFileEntry->getName().str();

we could get the canonical path from `onMainAST` callback (the first parameter 
`PathRef Path`).



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:60
+class HighlightingDiffer {
+  std::map> PrevHighlightingsMap;
+  std::mutex PrevMutex;

nit: llvm::StringMap.

I think this map is storing all file highlightings, maybe call it 
`FileToHighlightings`?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74
+  std::vector>>
+  diffHighlightings(const ParsedAST ,
+std::vector Highlightings);

this method does two things, 
1) calculate the diff between old and new
2) replace the old highlighting with new highlightings

but the name just reflects 1).

I think we just narrow it to 2) only:

```
// calculate the new highlightings, and return the old one.
std::vector updateFile(PathRef Path, 
std::vector NewHighlightings);

// to get diff, in `onMainAST`
diffHighlightings(differ.updateFile(Path, NewHighlightings), NewHighlightings);
```




Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:82
+  std::vector>>
+  diffHighlightings(ArrayRef Highlightings,
+ArrayRef PrevHighlightings);

this utility method doesn't use any internal member of the class, we could pull 
it out or make it static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[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:172
+  const HighlightingToken ) {
+  return Lhs.R.start.line == Rhs.R.start.line
+ ? Lhs.R.start.character < Rhs.R.start.character

sammccall wrote:
> I think this is just Lhs.R.start < Lhs.R.end
> 
> (if it's not, we should add the `operator<`)
> 
> is it enforced somewhere that you don't have two highlights at the same spot?
It's not enforced anywhere, it depends on how the  HighlightingTokenCollector 
is implemented, it should not currently take any multiples of tokens at the 
same spot.

Even if there are tokens at the same location it should still satisfy Compare 
though?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:190
+  // highlightings for old ASTs)
+  std::lock_guard Lock(PrevMutex);
+  // The files are different therefore all highlights differ.

sammccall wrote:
> holding the lock while doing all the diffing seems dubious
> 
> You could reasonably put the cache as a map in 
> ClangdServer, then you don't have to deal with not having the cache for the 
> current file.
> 
> You'd need to lock the map itself, but note that no races on updating 
> individual entries are possible because onMainAST is called synchronously 
> from the (per-file) worker thread.
I did not know onMainAST was called synchronously per file. 
I feel like after a while a lot of memory is going to be consumed by keeping 
ASTs for files that were closed long ago in the cache (say when you've opened 
1000 files in a session, all those files will have one Highlightings entry here)
Could solve that with LRU (just keep a set of pair 
(unix time of adding/editing to cache and filename)... would actually have to 
be a struct, would need to make the equal operator check the filename only)

But maybe that isn't a big concern/a concern at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

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

Removed unused code snippets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,7 +29,9 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple,
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
@@ -46,9 +48,43 @@
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {std::move(AST), ActualTokens, ExpectedTokens};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(std::ignore, ActualTokens, ExpectedTokens) =
+  getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(
+const std::vector ,
+const std::vector ,
+std::vector>> ) {
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector>>
+  ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto  : ExpectedLines) {
+std::sort(LineTokens.second.begin(), LineTokens.second.end());
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  // The UnorderedElementsAreArray only checks that the top level vector
+  // is unordered. The vectors in the pair must be in the correct order.
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+std::sort(ActualDiffed[I].second.begin(), ActualDiffed[I].second.end());
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -145,7 +181,9 @@
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
 void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+PathRef File,
+std::vector>>
+Highlightings) override {
   ++Count;
 }
   };
@@ -170,21 +208,148 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector>> Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector<
+  std::pair, std::pair>>
+  TestCases{{{},
+ {
+ R"cpp(
+int $Variable[[A]]
+double $Variable[[B]];
+struct $Class[[C]] {};
+  )cpp",
+ R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"}},
+{{5},
+ {
+ R"cpp(
+  struct $Class[[Alpha]] {
+double SomeVariable = 9483.301;
+  };
+  struct $Class[[Beta]] {};
+  int $Variable[[A]] = 121;
+  $Class[[Alpha]] $Variable[[Var]];
+  )cpp",
+ R"cpp(
+  struct Alpha {
+double SomeVariable = 9483.301;
+  };
+  struct Beta   {}; // Some comment
+  intA = 121;
+  $Class[[Beta]] 

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

jvikstrom updated this revision to Diff 209225.
jvikstrom marked 5 inline comments as done.
jvikstrom added a comment.

Made diffing function shorter, added multiple previous highlighting entries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,7 +29,9 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple,
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
@@ -46,9 +48,43 @@
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {std::move(AST), ActualTokens, ExpectedTokens};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(std::ignore, ActualTokens, ExpectedTokens) =
+  getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
+void checkDiffedHighlights(
+const std::vector ,
+const std::vector ,
+std::vector>> ) {
+  std::map> ExpectedLines;
+  for (const HighlightingToken  : ExpectedTokens)
+ExpectedLines[Token.R.start.line].push_back(Token);
+  std::vector>>
+  ExpectedLinePairHighlighting;
+  for (int Line : EmptyLines)
+ExpectedLinePairHighlighting.push_back({Line, {}});
+  for (auto LineTokens : ExpectedLines) {
+std::sort(LineTokens.second.begin(), LineTokens.second.end());
+ExpectedLinePairHighlighting.push_back(
+{LineTokens.first, LineTokens.second});
+  }
+
+  // The UnorderedElementsAreArray only checks that the top level vector
+  // is unordered. The vectors in the pair must be in the correct order.
+  for (unsigned I = 0, End = ActualDiffed.size(); I < End; ++I)
+std::sort(ActualDiffed[I].second.begin(), ActualDiffed[I].second.end());
+
+  EXPECT_THAT(ActualDiffed,
+  testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+}
+
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
 R"cpp(
@@ -145,7 +181,9 @@
 
 void onDiagnosticsReady(PathRef, std::vector) override {}
 void onHighlightingsReady(
-PathRef File, std::vector Highlightings) override {
+PathRef File,
+std::vector>>
+Highlightings) override {
   ++Count;
 }
   };
@@ -170,21 +208,148 @@
 return Pos;
   };
 
-  std::vector Tokens{
-  {HighlightingKind::Variable,
-Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
-  {HighlightingKind::Function,
-Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
-  {HighlightingKind::Variable,
-Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+  std::vector>> Tokens{
+  {3,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+{HighlightingKind::Function,
+ Range{CreatePosition(3, 4), CreatePosition(3, 7),
+  {1,
+   {{HighlightingKind::Variable,
+ Range{CreatePosition(1, 1), CreatePosition(1, 5)};
   std::vector ActualResults =
   toSemanticHighlightingInformation(Tokens);
   std::vector ExpectedResults = {
-  {1, "AQAEAAA="},
-  {3, "CAAEAAAEAAMAAQ=="}};
+  {3, "CAAEAAAEAAMAAQ=="}, {1, "AQAEAAA="}};
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector<
+  std::pair, std::pair>>
+  TestCases{{{},
+ {
+ R"cpp(
+int $Variable[[A]]
+double $Variable[[B]];
+struct $Class[[C]] {};
+  )cpp",
+ R"cpp(
+int A;
+double B;
+struct C {};
+  )cpp"}},
+{{5},
+ {
+ R"cpp(
+  struct $Class[[Alpha]] {
+double SomeVariable = 9483.301;
+  };
+  struct $Class[[Beta]] {};
+  int $Variable[[A]] = 121;
+  $Class[[Alpha]] $Variable[[Var]];
+  )cpp",
+ R"cpp(
+  struct Alpha {
+double SomeVariable = 9483.301;
+  };
+  

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:172
+  const HighlightingToken ) {
+  return Lhs.R.start.line == Rhs.R.start.line
+ ? Lhs.R.start.character < Rhs.R.start.character

I think this is just Lhs.R.start < Lhs.R.end

(if it's not, we should add the `operator<`)

is it enforced somewhere that you don't have two highlights at the same spot?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:190
+  // highlightings for old ASTs)
+  std::lock_guard Lock(PrevMutex);
+  // The files are different therefore all highlights differ.

holding the lock while doing all the diffing seems dubious

You could reasonably put the cache as a map in ClangdServer, 
then you don't have to deal with not having the cache for the current file.

You'd need to lock the map itself, but note that no races on updating 
individual entries are possible because onMainAST is called synchronously from 
the (per-file) worker thread.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+  // PrevHighlightings and Highlightings.
+  int I = 0, PI = 0, EndI = Highlightings.size(),
+  EndP = PrevHighlightings.size();

Whatever you do about storage, please pull out the diff(old, new) function so 
you can unit test it.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+  // PrevHighlightings and Highlightings.
+  int I = 0, PI = 0, EndI = Highlightings.size(),
+  EndP = PrevHighlightings.size();

sammccall wrote:
> Whatever you do about storage, please pull out the diff(old, new) function so 
> you can unit test it.
(llvm uses `unsigned` for indices. It's a terrible idea, but it's used fairly 
consistently...)



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:209
+  EndP = PrevHighlightings.size();
+  while (I < EndI && PI < EndP) {
+const HighlightingToken  = Highlightings[I];

I can believe this is correct, but it's hard to verify as it's a bit monolithic 
and... index-arithmetic-y.

could you factor out something like:

```
using TokRange = ArrayRef;

// The highlights for the current line.
TokRange OldLine = {PrevHighlightings.data(), 0}, NewLine = 
{Highlightings.data(), 0};
// iterate over lines until we run out of data
for (unsigned Line = 0; OldLine.end() < PrevHighlightings.end() || 
NewLine.end() < PrevHighlightings.end(); ++Line) {
  // Get the current line highlights
  OldLine = getLineRange(PrevHighlightings, Line, OldLine);
  NewLine = getLineRange(Highlightings, Line, NewLine);
  if (OldLine != NewLine)
emitLine(NewLine, Line);
}

// get the highlights for Line, which follows PrevLine
TokRange getLineRange(TokRange AllTokens, unsigned Line, TokRange PrevLine) {
  return makeArrayRef(PrevLine.end(), AllTokens.end()).take_while( Tok => 
Tok.R.start.line == Line);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for bringing this up and implementing it.

I haven't looked into the details of the patch, just some high-level comments:

- the scope of the patch seems a bit unclear to me, what's the problem we are 
trying to solve?
- the patch looks like calculating the diff/delta highlightings (pre 
highlightings vs. new highlightings) from a server-only perspective, which 
seems incorrect. At least we should make sure that LSP client and server share 
the same understanding of source diff change, otherwise client may render the 
delta information in a wrong way. To do that, I guess we may start from the 
`DidChangeTextDocument` notification (where the client sends source changes to 
server);
- I'm not sure that we should start implement this at the moment -- I don't see 
that we will get much performance gain, we save some traffic cost between LSP 
client and server, but at the same time we are spending more CPU resource to 
compute highlighting diff in server side. Maybe there are some other clever 
ways (like traversing the decls in source diff change);

We probably need more investigations, like trying the current implementation on 
some real-project files in a real client (e.g. theia) to figure out and 
understand whether the latency is a real issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

Added a class for diffing highlightings and removing duplicate lines. 
Integrated into the highlighting generation flow. Only works correctly if all 
tokens are on a single line. Also returns empty lines if the IDE should remove 
previous highlightings on a line.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64475

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  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
@@ -29,14 +29,17 @@
   return Tokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+std::tuple,
+   std::vector>
+getHighlightingsAnnotated(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map KindToString{
   {HighlightingKind::Variable, "Variable"},
   {HighlightingKind::Function, "Function"},
   {HighlightingKind::Class, "Class"},
-  {HighlightingKind::Enum, "Enum"}};
+  {HighlightingKind::Enum, "Enum"},
+  {HighlightingKind::Empty, "Empty"}};
   std::vector ExpectedTokens;
   for (const auto  : KindToString) {
 std::vector Toks = makeHighlightingTokens(
@@ -45,6 +48,14 @@
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
+  return {std::move(AST), ActualTokens, ExpectedTokens};
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  std::vector ActualTokens;
+  std::vector ExpectedTokens;
+  std::tie(std::ignore, ActualTokens, ExpectedTokens) =
+  getHighlightingsAnnotated(Code);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
@@ -161,6 +172,68 @@
   EXPECT_EQ(ActualResults, ExpectedResults);
 }
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  std::vector> TestCases{{
+   R"cpp(
+struct $Class[[Alpha]] {
+  double SomeVariable = 9483.301;
+};
+struct $Class[[Beta]] {};
+int $Variable[[A]] = 121;
+$Class[[Alpha]] $Variable[[Var]];
+  )cpp",
+   R"cpp(
+struct Alpha {
+  double SomeVariable = 9483.301;
+};
+struct Beta   {}; // Some changes that don't change the tokens' position.
+intA$Empty[[]] = 121;
+$Class[[Beta]] $Variable[[Var]];
+  )cpp"},
+   {
+   R"cpp(
+  int $Variable[[A]] = 121; int $Variable[[B]];
+)cpp",
+   R"cpp(
+  intA = 121; int $Variable[[B]];
+)cpp"},
+   {
+   R"cpp(
+  int $Variable[[A]] = 213;
+)cpp",
+   R"cpp(
+  int C = 312;
+  int $Variable[[B]] = 213;
+)cpp",
+   R"cpp(
+  int C = 213;
+  int B = 412;
+)cpp"},
+   {
+   R"cpp(
+  int $Variable[[A]];
+)cpp",
+   R"cpp(
+  int $Variable[[A]]; int $Variable[[B]];
+)cpp"}};
+
+  for (auto Test : TestCases) {
+HighlightingDiffer Differ;
+for (const auto  : Test) {
+  ParsedAST AST =
+  TestTU::withCode("").build(); // Can't default construct a ParsedAST.
+  std::vector CompleteTokens;
+  std::vector ExpectedTokens;
+  std::tie(AST, CompleteTokens, ExpectedTokens) =
+  getHighlightingsAnnotated(Code);
+  std::vector DiffedTokens =
+  Differ.diffHighlightings(AST, CompleteTokens);
+  EXPECT_THAT(DiffedTokens,
+  testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- clang-tools-extra/clangd/test/semantic-highlighting.test
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -34,6 +34,21 @@
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---