This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Mar 6 2019, 8:52 AM.

Event Timeline

ldionne created this revision.Mar 6 2019, 8:52 AM
mclow.lists accepted this revision.Mar 6 2019, 9:03 AM

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"???

This revision is now accepted and ready to land.Mar 6 2019, 9:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 9:06 AM

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.

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

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.

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.

[...]

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.

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

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.

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.

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