[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comments at this point; but before landing it please

- wait for a second OK from the "libc++" reviewer
- make sure CI is green



================
Comment at: libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp:47-50
+#define CHECK_ALIGNMENT(T)                                                                                             \
+  do {                                                                                                                 \
+    typedef T type;                                                                                                    \
+    atomic_test<type> t;                                                                                               \
----------------
Please leave the original indentation here, and also on line 38 above, and also on all the lines below this one.
If you're doing this because clang-format recommended it, please ignore clang-format.
It looks to me like the //only// diff that should be in this file is just adding `-Wno-psabi` on line 11 and all these other diffs are bogus, right?


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp:165-215
     {
         std::atomic_bool obj(true);
         assert(obj == true);
-        std::atomic_init(&obj, false);
-        assert(obj == false);
-        std::atomic_init(&obj, true);
-        assert(obj == true);
         bool b0 = obj.is_lock_free();
         (void)b0; // to placate scan-build
         obj.store(false);
         assert(obj == false);
----------------
FWIW, one could actually replace these 50 lines with
```
static_assert(std::is_same<std::atomic_bool, std::atomic<bool> >::value, "");
```
You don't have to do that in this PR, but I wouldn't fault you if you did. :)


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp:29
+  }
+}
----------------
Nice!
Nit: please make `main`'s signature `int main(int, char**)` and add `return 0;` at the end. We do this on all `main`s, for some reason related to testing on embedded platforms.


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp:74-78
 #ifdef _LIBCPP_VERSION // libc++ is nonconforming
         std::atomic_fetch_add<X>(&t, 0);
 #else
         std::atomic_fetch_add<T>(&t, 0);
 #endif // _LIBCPP_VERSION
----------------
Attn: @Mordante! These bogusly-template-argumented calls are relevant to D103983, right?

(This diff LGTM. Just cross-referencing it with @Mordante's PR, which will merge-conflict with it.)

These tests are utterly bogus, btw. Basically what they're doing is initializing an `atomic<int*>` with `(int*)(4)`, and then fetch_add'ing `2` to it, and then asserting that the result is `(int*)(12)`. This is entirely UB. They should be rewritten to use an array of `T` or something. But that's out of scope for this PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769