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

Johan Vikström via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 07:54:03 PDT 2019

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list