[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 <https://reviews.llvm.org/D64475#1593481>, @ilya-biryukov wrote:

> The fix for a race condition on remove has landed in rL366577 <https://reviews.llvm.org/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<int> 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