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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

LGTM, but there's one comment not addressed yet. Please fix that before landing the patch.



================
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;                                                                                               \
----------------
tambre wrote:
> Quuxplusone wrote:
> > 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?
> > 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?
> 
> Correct, done. Would be nice if someone either ran clang-format on the codebase or the configuration better matched the actual mandated style.
The problem is the version of clang-format used can't properly format the entire code base. In C++03 mode it breaks C++20 and in C++20 mode it breaks C++03. Probably we can use clang-format on our codebase once we use clang-format 13.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/throw.pass.cpp:29
+  }
+}
----------------
Quuxplusone wrote:
> 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.
This comment hasn't been addressed yet.


================
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
----------------
Quuxplusone wrote:
> 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.
Correct this is what I'm fixing in D103983.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769