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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: include/atomic:603
+
+#endif
 
----------------
jwakely wrote:
> zoecarver wrote:
> > jfb wrote:
> > > jwakely wrote:
> > > > jfb wrote:
> > > > > I think you want to keep the old `typedef enum memory_order` before C++20, and only enable the new thing in C++20 and later.
> > > > That's what we did for libstdc++.
> > > You did what I'm suggesting, or what @zoecarver did? 🤔
> > Changing how `memory_order ` is defined changes how some of the code below is written -- if it is defined differently for different version then the code below also needs to be changed for different versions (actually a `static_cast` would work in both cases but it is only necessary in the latter). 
> We did what you suggested: keeping it unchanged for C++11/14/17 and only making it a scoped enumeration (and adding the new enumerators) for C++20.
> 
> And we cast to `int` unconditionally, because as zoecarver said, that always works, even if the cast is redundant for C++11/14/17.
Although I know it's not conforming. I would prefer to make it a scoped enum retroactively. In the past 3 months, I've had 3 separate users get burned by the implicit conversion to int. 



================
Comment at: include/atomic:582
+
+enum class memory_order
+{
----------------
I would like to see an explicit underlying type declared here.


================
Comment at: include/atomic:601
 } memory_order;
 
+#endif
----------------
And we should static assert the underlying type of this matches the type we declare in C++17.


================
Comment at: include/atomic:602
 
+#endif
+
----------------
Comment on `#endif`


================
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));}
----------------
Instead of casting to `int`, I would rather to a typedef for the real underlying type.

I'm probably just being neurotic.


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

https://reviews.llvm.org/D58201