[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.req/atomic_compare_exchange_weak_explicit.pass.cpp:11
 // XFAIL: !non-lockfree-atomics
 //  ... assertion fails line 38
 
----------------
Quuxplusone wrote:
> Pre-existing: this comment isn't right, and remains not-right after your PR changes the line numbers. I didn't look to find out which line number it should have said.
I'll remove this line and in the other places.


================
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:
> 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`.)


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_wait_explicit.pass.cpp:60
     });
-    std::atomic_wait(&vt, T(2));
+    std::atomic_wait_explicit(&vt, T(2), std::memory_order_seq_cst);
     t2.join();
----------------
Quuxplusone wrote:
> Would it make sense to test any other memory orders? I assume there's nothing we can really do in a test to verify that the codegen uses the "right" memory order, though, so whatever.
It might make sense, but I noticed the other tests also only test `std::memory_order_seq_cst`. In that case we should look at that for all these tests not only this one. Since we have some ARM build bots it would be interesting to test this. (On x86 the CPU often has a stronger memory order than the requested memory order.)

But I see that out of scope for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103765