[libcxx-commits] [PATCH] D103765: [libc++] Update atomic synopsis and tests.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_one.pass.cpp:51
+    {
+      volatile A a(T(1));
+      std::thread t = support::make_test_thread([&]() {
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Arguably, it'd be better to keep using `2` and `4` for this second set of tests, just to guard against the remote possibility that something goes wrong when the stack frame reuses the same memory address for both `a` variables.
> > > 
> > > Also, it looks like you removed the test coverage of `std::atomic_wait(&t, T(some-other-value))`, which is expected to return immediately as a no-op.
> > The tests for `atomic_wait` and `atomic_wait_explicit` test the nop. This test is for
> > `atomic_notify_one` so here there's no need to test this nop. (This test is based on the `atomic_wait`.)
> Ah, I get it. OK.
> Might be a good idea to insert `assert(a.load() == T(3))` before `t.join()`, just for paranoia. Cf. line 46 of `atomic_notify_all.pass.cpp`.
It might be paranoia here, but the `atomic_wait` tests also don't do this test. I'll add them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103765