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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 07:26:10 PDT 2019

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
+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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list