[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 22 13:30:47 PST 2020


sammccall added a comment.

In D93726#2468855 <https://reviews.llvm.org/D93726#2468855>, @qchateau wrote:

> LGTM
>
> Small nits:
>
> - `operator()` is not `const` but `Next` is `mutable`, seems to me you intended to have `operator()` as `const`

Oops, I originally did plan to make `operator()` const, but then decided it'd be clearer for the caller to make PeriodicThrottler `mutable` instead. (Because pretending it doesn't have state is confusing).
Removed mutable from `Next`.

> - memory orders for the atomic operations can probably be downgraded to `std::memory_order_acquire`/`std::memory_order_acq_rel`. I think the first load can even be `relaxed` but that I'm always careful with these

Done. I have to confess I often pretend memory orders other than seq_cst don't exist just to keep the cognitive load down, but this case seems clear/isolated enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93726



More information about the cfe-commits mailing list