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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

LGTM from my side



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+    Old = std::move(FileToHighlightings[File]);
+    FileToHighlightings[File] = Highlightings;
+  }
----------------
jvikstrom wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > NIT: avoid copying (from `Highlightings` into the map) under a lock, make the copy outside the lock instead
> > I think we can even avoid copy, since we only use the `Highlightings` for calculating the diff.
> > 
> > Maybe just 
> > 
> > ```
> > {
> >     std::lock_guard<std::mutex> Lock(HighlightingsMutex);
> >     Old = std::move(FileToHighlightings[File]);
> > }
> > // calculate the diff.
> > {  
> >      std::lock_guard<std::mutex> Lock(HighlightingsMutex);
> >      FileToHighlightings[File] = std::move(Highlightings);
> > }
> > ```
> I think I changed this to only lock once (and copy instead) at the same time me and @ilya-biryukov were talking about the race condition (?)
It means there's a window where nobody can access the highlightings for this file. Which is probably fine, we really do use this only for computing the diff and nothing else.

But doing the copy shouldn't matter here either, so leaving as is also looks ok from my side.

If you decide to avoid an extra copy, please add comments to `FileToHighlightings` (e.g. `only used to compute diffs, must not be used outside onHighlightingsReady and didRemove`).
It is declared really close to `FixItsMap` and looks very similar, but the latter is used in completely different ways; that could lead to confusion.

Don't remember exact details about race conditions, but we should be good now that it's called inside `Publish()`


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408
+}
+
 } // namespace
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Could you also add a separate test that checks diffs when the number of lines is getting smaller (i.e. taking `NLines` into account)
> > I would expect this to be better handled outside `checkDiffedHighlights` to avoid over-generalizing this function. But up to you.
> That's what this test does: 
> 
> ```
> {
>                     R"(
>         $Class[[A]]
>         $Variable[[A]]
>         $Class[[A]]
>         $Variable[[A]]
>       )",
>                     R"(
>         $Class[[A]]
>         $Variable[[A]]
>       )"},
> ```
> 
> But do you mean I should do a testcase like:
> 
> 
> 
> ```
> {
>                     R"(
>         $Class[[A]]
>         $Variable[[A]]
>         $Class[[A]]
>         $Variable[[A]]
>       )",
>                     R"(
>         $Class[[A]]
>         $Variable[[A]]
>         $Class[[A]]
>         $Variable[[A]]
>       )"},
> ```
> And just set NLines = 2 instead when doing the diffing?
> 
Ah, the test actually needs the set of changed lines to be **exactly** the same as marked lines.
In that case, all is good, I've just missed this detail.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64475/new/

https://reviews.llvm.org/D64475