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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 6 inline comments as done.
zoecarver added inline comments.


================
Comment at: include/atomic:603
+
+#endif
 
----------------
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). 


================
Comment at: include/atomic:932
       _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m)
-        {__c11_atomic_store(&__a_, __d, __m);}
+        {__c11_atomic_store(&__a_, __d, static_cast<int>(__m));}
     _LIBCPP_INLINE_VISIBILITY
----------------
jfb wrote:
> jwakely wrote:
> > jfb wrote:
> > > Hmm this is unfortunate. Can we change the builtins to accept the enum? I guess that would make mixing compiler / stdlib versions harder... So I think what you did here is best.
> > And we also use casts to `int` in libstdc++. Changing the builtins wouldn't help if somebody used new libstdc++ headers with an old Clang or icc that only accepts ints.
> Right, it makes me slightly sad at how ugly the code is, but it was already ugly to start with 😉
Not to mention how much of a pain it is...


================
Comment at: src/experimental/memory_resource.cpp:116
         return _VSTD::atomic_load_explicit(
-            &__res, memory_order::memory_order_acquire);
+            &__res, memory_order::acquire);
     }
----------------
jfb wrote:
> Here I'd keep the old spelling, so it's compatible before C++20.
Good call, I should probably also do that in `atomic` right?


================
Comment at: test/std/atomics/atomics.order/memory_order.pass.cpp:29
-    assert(std::memory_order_acq_rel == 4);
-    assert(std::memory_order_seq_cst == 5);
     std::memory_order o = std::memory_order_seq_cst;
----------------
jfb wrote:
> I think you want to keep this test, and add your new ones in a separate test, conditional to C++20.
Will do.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58201