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



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

Reply via email to