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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
