[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<std::mutex> 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<std::mutex> 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<std::mutex> 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<HighlightingToken> 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<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings,
+                  ArrayRef<HighlightingToken> 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