[libcxx-commits] [PATCH] D58201: Make std::memory_order an enum class (P0439R0)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: include/atomic:932-935
     _LIBCPP_INLINE_VISIBILITY
     void store(_Tp __d, memory_order __m = memory_order_seq_cst) _NOEXCEPT
       _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m)
+        {__c11_atomic_store(&__a_, __d, static_cast<int>(__m));}
----------------
EricWF wrote:
> Instead of casting to `int`, I would rather to a typedef for the real underlying type.
> 
> I'm probably just being neurotic.
Just a note that now we're casting to the underlying type (`unsigned int`), not `int`.


================
Comment at: include/atomic:25
+ 
+enum class memory_order: unsigned
+{
----------------
This should say `enum memory_order : unspecified     // enum class in C++20`.

We do not encode libc++ specific properties in the synopsis as far as I'm aware, and the standard does not mention anything specific here.


================
Comment at: include/atomic:617
+
+using memory_order_t = _VSTD::underlying_type<memory_order>::type; // unsigned
+
----------------
zoecarver wrote:
> ldionne wrote:
> > I didn't see this in the proposal -- did I miss it or was this added on the side? If it's not part of the proposal, it shouldn't be part of our API, and you should use a mangled name if you really need to designate this type at all.
> If I understand correctly, @EricWF asked me to create a typedef for this so we can `static_cast` to that later. Forgot to mangle it. 
> 
> I am not sure we need a typedef but I don't feel strongly one way or another. 
We can't add non-reserved names to our API. Either you don't introduce a typedef, or you introduce a typedef with a name like `__memory_order_t` (I suggest `__memory_order_underlying_t` if you want to keep a typedef).


================
Comment at: test/std/atomics/atomics.order/memory_order_new.pass.cpp:12
+#include <atomic>
+#include <cassert>
+
----------------
Not needed.


================
Comment at: test/std/atomics/atomics.order/memory_order_new.pass.cpp:16
+{
+    assert(std::memory_order_relaxed == std::memory_order::relaxed);
+    assert(std::memory_order_consume == std::memory_order::consume);
----------------
`static_assert`


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

https://reviews.llvm.org/D58201