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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Requesting changes, but basically this looks like an improvement and I don't actually care to see it again before it lands. :)



================
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
 
----------------
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.


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp:11
 // XFAIL: !non-lockfree-atomics
 //  ... fails assertion line 31
 
----------------
Ditto here.


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange_explicit.pass.cpp:11
 // XFAIL: !non-lockfree-atomics
 //  ... assertion fails line 32
 
----------------
Ditto here. (Etc. etc.)


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


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103765