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

[JF Bastien via Phabricator via libcxx-commits]

jfb added inline comments.


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


================
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
----------------
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.


================
Comment at: src/experimental/memory_resource.cpp:116
         return _VSTD::atomic_load_explicit(
-            &__res, memory_order::memory_order_acquire);
+            &__res, memory_order::acquire);
     }
----------------
Here I'd keep the old spelling, so it's compatible before C++20.


================
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;
----------------
I think you want to keep this test, and add your new ones in a separate test, conditional to C++20.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58201