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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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([&]() {
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103765