hokein accepted this revision.
hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125
+ Old = std::move(FileToHighlightings[File]);
+ FileToHighlightings[File] = Highlightings;
+ }
----------------
ilya-biryukov wrote:
> 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()`
I'm fine with the current state, avoiding the copy cost would make the code
here a bit mess...
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
