[libcxx-commits] [PATCH] D97044: [libc++] [C++2b] [P0943] Add stdatomic.h header.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: timshen.
ldionne added a comment.
This revision now requires changes to proceed.

Can you rebase onto `main` and go through https://libcxx.llvm.org/Contributing.html?



================
Comment at: libcxx/include/stdatomic.h:133
+
+#define _Atomic(T) _VSTD::atomic<T>
+
----------------
curdeius wrote:
> Should I use `::_VSTD::atomic<T>` or should we go for changing `_VSTD` to `::std`? Or is it not worth the trouble?
Use `_VSTD` until we decide what we do with that, for consistency.


================
Comment at: libcxx/include/stdatomic.h:135
+
+using _VSTD::memory_order;
+using _VSTD::memory_order_relaxed;
----------------
Can you use `_LIBCPP_USING_IF_EXISTS` everywhere?


================
Comment at: libcxx/test/std/atomics/stdatomic.h.syn/types.pass.cpp:21
+// #define ATOMIC_CHAR_LOCK_FREE see below
+// TODO: No ATOMIC_CHAR8_T_LOCK_FREE, but atomic_char8_t is present in synopis?? NB that atomic_char8_t is not in C11. See http://eel.is/c++draft/stdatomic.h.syn.
+// #define ATOMIC_CHAR16_T_LOCK_FREE see below
----------------
curdeius wrote:
> Anyone has an idea about this? Is this an omission in the standard (given that it includes `atomic_char8_t`) or it should match C11 and so omit both `ATOMIC_CHAR8_T_LOCK_FREE` and `atomic_char8_t`?
GCC does define `ATOMIC_CHAR8_T_LOCK_FREE`. I think this is likely an oversight in the spec.

@timshen Would you agree?

@curdeius I think we should add both `ATOMIC_CHAR8_T_LOCK_FREE` and `atomic_char8_t`.


================
Comment at: libcxx/test/std/atomics/stdatomic.h.syn/types.pass.cpp:175
+
+  static_assert((std::is_same_v<std::atomic<int8_t>,   ::atomic_int8_t>));
+  static_assert((std::is_same_v<std::atomic<uint8_t>,  ::atomic_uint8_t>));
----------------
You don't need double-parens here and elsewhere, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97044