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