[libc++] Do not specify the underlying type of memory_order

[ldionne]

Summary

This breaks ABI for folks using -fshort-enums, and does not really buy
us anything.

http://llvm.org/PR40977

[mclow.lists]

LGTM. I wish we could keep a static_assert on line 616, but I don't know what to assert. "sizeof(__memory_order_underlying_t) didn't change"???

[zoecarver]

Seems good to me. I remember @EricWF saying

And we should static assert the underlying type of this matches the type we declare in C++17.

and

I would like to see an explicit underlying type declared here.

though. He might have a point.

[mclow.lists]

In D59029#1419972, @zoecarver wrote:

Seems good to me. I remember @EricWF saying

And we should static assert the underlying type of this matches the type we declare in C++17.

and

I would like to see an explicit underlying type declared here.

though. He might have a point.

I don't see how to do both. - match the C++17 size (which can change), and be explicit.

I liked the static_assert that we had before - it found us a bug: https://bugs.llvm.org/show_bug.cgi?id=40977

[ldionne]

In D59029#1419974, @mclow.lists wrote:

In D59029#1419972, @zoecarver wrote:

Seems good to me. I remember @EricWF saying

And we should static assert the underlying type of this matches the type we declare in C++17.

and

I would like to see an explicit underlying type declared here.

though. He might have a point.

I don't see how to do both. - match the C++17 size (which can change), and be explicit.

I liked the static_assert that we had before - it found us a bug: https://bugs.llvm.org/show_bug.cgi?id=40977

Well, it found that we had introduced a bug with that revision -- now there's nothing to static_assert anymore, since we're not saying anything about the underlying type. It's entirely possible that @EricWF had something else in mind -- if so, he can comment here and I can change the code again.

[ldionne]

Actually, the only static_assert I can conceive here would be this:

#if _LIBCPP_STD_VER > 17

typedef enum __cpp17_memory_order {
  __cpp17_memory_order_relaxed, __cpp17_memory_order_consume, __cpp17_memory_order_acquire,
  __cpp17_memory_order_release, __cpp17_memory_order_acq_rel, __cpp17_memory_order_seq_cst
} __cpp17_memory_order;

enum class memory_order {
  relaxed, consume, acquire, release, acq_rel, seq_cst
};

static_assert(is_same<underlying_type<__cpp17_memory_order>::type, 
                      underlying_type<memory_order>::type>::value,
              "The C++20 and C++17 memory order enumerations should have the same underlying type");

#else

typedef enum memory_order {
  memory_order_relaxed, memory_order_consume, memory_order_acquire,
  memory_order_release, memory_order_acq_rel, memory_order_seq_cst
} memory_order;

#endif // _LIBCPP_STD_VER > 17

TBH, I don't think there's a lot of value in this. However, if we care that much about this, then I'd probably revert my previous position of not back-porting the enum class behavior to pre-C++20. IOW, instead of adding this complexity, I'd just make the definition the same in C++17 and C++20, and we'd be non-conforming in C++17.

[ldionne]

In D59029#1420019, @ldionne wrote:

[...]

TBH, I don't think there's a lot of value in this. However, if we care that much about this, then I'd probably revert my previous position of not back-porting the enum class behavior to pre-C++20. IOW, instead of adding this complexity, I'd just make the definition the same in C++17 and C++20, and we'd be non-conforming in C++17.

Or we could do that in a test only, as Marshall suggested.

[mclow.lists]

In D59029#1420019, @ldionne wrote:

Actually, the only static_assert I can conceive here would be this:

[snip]

TBH, I don't think there's a lot of value in this. However, if we care that much about this, then I'd probably revert my previous position of not back-porting the enum class behavior to pre-C++20. IOW, instead of adding this complexity, I'd just make the definition the same in C++17 and C++20, and we'd be non-conforming in C++17.

I think we can put the static_assert in a test, instead. Something like:

typedef enum Fooby { six dummy values };
static_assert(sizeof(std::memory_order) == sizeof(Fooby), "" );

and that should pass on both C++17 and 20 with and without fshort-enums

[ldionne]

I'm going to add a test. I'm not sure whether we should be checking that the underlying types are the same, or that they have the same size. Scoped and unscoped enumerations have different underlying types in GCC and Clang but I'm not 100% sure the difference is intended (it probably is). I've asked a question to CWG and I'll update this when I know for sure.

[zoecarver]

I don't feel strongly one way or another. Putting it in the tests is probably fine, but we should make sure they pass with both fshort-enums and without. If we decide on that, I am happy to implement it in another test.

[zoecarver]

Seems like both this patch and D59063 are trying to solve the same problems.

libcxx/include/atomic

#define _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__m, __f) \
   _LIBCPP_DIAGNOSE_WARNING(__f == memory_order_release || \
                            __f == memory_order_acq_rel,   \
                         "memory order argument to atomic operation is invalid")
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if _LIBCPP_STD_VER > 17
 
-enum class memory_order: unsigned {
+enum class memory_order {
   relaxed, consume, acquire, release, acq_rel, seq_cst
 };
 
 inline constexpr auto memory_order_relaxed = memory_order::relaxed;
 inline constexpr auto memory_order_consume = memory_order::consume;
 inline constexpr auto memory_order_acquire = memory_order::acquire;
 inline constexpr auto memory_order_release = memory_order::release;
 inline constexpr auto memory_order_acq_rel = memory_order::acq_rel;
 inline constexpr auto memory_order_seq_cst = memory_order::seq_cst;
 
-static_assert((is_same<underlying_type<memory_order>::type,
-         unsigned>::value), "Underlying type differs from unsigned unexpectedly");
-
 #else
 
 typedef enum memory_order {
   memory_order_relaxed, memory_order_consume, memory_order_acquire,
   memory_order_release, memory_order_acq_rel, memory_order_seq_cst
 } memory_order;
 
-static_assert((is_same<underlying_type<memory_order>::type,
-         unsigned>::value), "Underlying type differs from unsigned unexpectedly");
-
 #endif // _LIBCPP_STD_VER > 17
 
-typedef underlying_type<memory_order>::type __memory_order_underlying_t; // unsigned
+typedef underlying_type<memory_order>::type __memory_order_underlying_t;
 
 
 #if defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) || \
   defined(_LIBCPP_ATOMIC_ONLY_USE_BUILTINS)
 
 // [atomics.types.generic]p1 guarantees _Tp is trivially copyable. Because
 // the default operator= in an object is not volatile, a byte-by-byte copy
 // is required.