Page MenuHomePhabricator

Make std::memory_order an enum class (P0439R0)
ClosedPublic

Authored by zoecarver on Feb 13 2019, 12:46 PM.

Details

Summary

Change memory_order to an enum class and fix all references to it (which expect it to implicitly cast to an int).

For reference: P0439R0.

Diff Detail

Repository
rL LLVM

Event Timeline

zoecarver created this revision.Feb 13 2019, 12:46 PM
jfb added inline comments.Feb 13 2019, 3:01 PM
include/atomic
603 ↗(On Diff #186721)

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.

932 ↗(On Diff #186721)

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.

src/experimental/memory_resource.cpp
116 ↗(On Diff #186721)

Here I'd keep the old spelling, so it's compatible before C++20.

test/std/atomics/atomics.order/memory_order.pass.cpp
29 ↗(On Diff #186721)

I think you want to keep this test, and add your new ones in a separate test, conditional to C++20.

jwakely added inline comments.Feb 13 2019, 3:07 PM
include/atomic
603 ↗(On Diff #186721)

That's what we did for libstdc++.

932 ↗(On Diff #186721)

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.

jfb marked an inline comment as done.Feb 13 2019, 3:10 PM
jfb added inline comments.
include/atomic
603 ↗(On Diff #186721)

You did what I'm suggesting, or what @zoecarver did? 🤔

932 ↗(On Diff #186721)

Right, it makes me slightly sad at how ugly the code is, but it was already ugly to start with 😉

zoecarver marked 6 inline comments as done.Feb 13 2019, 3:45 PM
zoecarver added inline comments.
include/atomic
603 ↗(On Diff #186721)

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

932 ↗(On Diff #186721)

Not to mention how much of a pain it is...

src/experimental/memory_resource.cpp
116 ↗(On Diff #186721)

Good call, I should probably also do that in atomic right?

test/std/atomics/atomics.order/memory_order.pass.cpp
29 ↗(On Diff #186721)

Will do.

jwakely added inline comments.Feb 13 2019, 4:15 PM
include/atomic
603 ↗(On Diff #186721)

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.

zoecarver updated this revision to Diff 186790.Feb 13 2019, 7:39 PM
zoecarver marked 4 inline comments as done.

Fix based on review and mark complete in status.

jfb added a comment.Feb 13 2019, 9:08 PM

Two small comments, LGTM otherwise (but would like one of the libc++ maintainers to sign off too).

include/atomic
33 ↗(On Diff #186790)

I'm not a fan of the huge comment at the top of files, but I assume you'll need to update it.

test/std/atomics/atomics.order/memory_order_new.pass.cpp
28 ↗(On Diff #186790)

This is redundant with the pre-existing test, I would drop it (and keep only lines 16-21).

EricWF added a subscriber: EricWF.Feb 14 2019, 12:17 AM
EricWF added inline comments.
include/atomic
603 ↗(On Diff #186721)

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.

582 ↗(On Diff #186790)

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

601 ↗(On Diff #186790)

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

602 ↗(On Diff #186790)

Comment on #endif

932–935 ↗(On Diff #186790)

Instead of casting to int, I would rather to a typedef for the real underlying type.

I'm probably just being neurotic.

jfb added inline comments.Feb 14 2019, 8:40 AM
include/atomic
603 ↗(On Diff #186721)

I'd rather not do this: WG21 would have made the paper a defect and applied its resolution retroactively. I think we should conform here, and we could instead teach clang-tidy to catch this issue.

zoecarver marked 4 inline comments as done.Feb 14 2019, 8:56 AM
zoecarver added inline comments.
include/atomic
603 ↗(On Diff #186721)

For what its worth, I would also rather not break the standard. What if we sort of compromised and left memory_order strongly typed but made all of memory_order_relaxed (etc.) ints so they could be backward compatible. I know it is not directly solving the issue you brought up but it would give a source of truth people could use and it would allow pre C++20 code to compile unmodified.

zoecarver marked an inline comment as done.Feb 14 2019, 9:03 AM

@EricWF I have resolved all your comments except for making memory_order scoped (which I can do after more discussion if still necessary).

include/atomic
603 ↗(On Diff #186721)

I guess the issue with that is things which are defined as memory_order still couldn't accept ints.

zoecarver marked 3 inline comments as done.Feb 14 2019, 9:07 AM
zoecarver updated this revision to Diff 186855.EditedFeb 14 2019, 9:10 AM
  • Add memeory_order_t
  • Declare underlying type (and check it)
  • Fix header comment
  • Check std version is greater than 17 (not 14)
  • Remove unnessisary tests
ldionne requested changes to this revision.Feb 15 2019, 9:19 AM
ldionne added inline comments.
include/atomic
603 ↗(On Diff #186721)

I'd rather not do this: WG21 would have made the paper a defect and applied its resolution retroactively. I think we should conform here, and we could instead teach clang-tidy to catch this issue.

I agree with this: I think we should conform since this is not a DR.

617 ↗(On Diff #186855)

I didn't see this in the proposal -- did I miss it or was this added on the side? If it's not part of the proposal, it shouldn't be part of our API, and you should use a mangled name if you really need to designate this type at all.

This revision now requires changes to proceed.Feb 15 2019, 9:19 AM
zoecarver marked 2 inline comments as done.Feb 15 2019, 10:31 AM
zoecarver added inline comments.
include/atomic
603 ↗(On Diff #186721)

I will keep it the same then?

Side question, what does DR stand for?

617 ↗(On Diff #186855)

If I understand correctly, @EricWF asked me to create a typedef for this so we can static_cast to that later. Forgot to mangle it.

I am not sure we need a typedef but I don't feel strongly one way or another.

jfb added inline comments.Feb 15 2019, 10:39 AM
include/atomic
603 ↗(On Diff #186721)

Louis and I would like to keep the new behavior for C++17 only, yes.

"Defect Report". They apply retroactively to previous standards, whereas papers usually only apply to the upcoming standard.

zoecarver marked an inline comment as done.Feb 16 2019, 11:33 AM
zoecarver added inline comments.
include/atomic
603 ↗(On Diff #186721)

Thanks for the definition. In that case, I won't update this patch to defect from the paper.

ldionne added inline comments.Feb 28 2019, 4:44 PM
include/atomic
932–935 ↗(On Diff #186790)

Just a note that now we're casting to the underlying type (unsigned int), not int.

25 ↗(On Diff #186855)

This should say enum memory_order : unspecified // enum class in C++20.

We do not encode libc++ specific properties in the synopsis as far as I'm aware, and the standard does not mention anything specific here.

617 ↗(On Diff #186855)

We can't add non-reserved names to our API. Either you don't introduce a typedef, or you introduce a typedef with a name like __memory_order_t (I suggest __memory_order_underlying_t if you want to keep a typedef).

test/std/atomics/atomics.order/memory_order_new.pass.cpp
12 ↗(On Diff #186855)

Not needed.

16 ↗(On Diff #186855)

static_assert

@ldionne Fixed, should be good to go. Thanks for the review :)

zoecarver updated this revision to Diff 188815.Feb 28 2019, 5:18 PM
ldionne accepted this revision.Feb 28 2019, 6:07 PM

This LGTM, however I'll wait before merging this because this will conflict with the non-lockfree atomic patch. It is unfortunate, but I'd rather rebase this one on top of the non-lockfree atomic one than the other way around. The non-lockfree atomic patch is much larger and complicated.

This revision is now accepted and ready to land.Feb 28 2019, 6:07 PM
zoecarver added a comment.EditedFeb 28 2019, 6:24 PM

No worries, it doesn't make a difference to me when/how you commit it. I am just glad I don't have to deal with the merge conflicts :P

Okay -- I've applied the other atomic patch, can you please rebase this one on top of the latest master? Sorry for the churn.

zoecarver updated this revision to Diff 189269.Mar 4 2019, 7:30 PM

That was fun :P

It seems like this patch no longer effects src/experimental/memory_resource.cpp other than that it should be good to go.

ldionne accepted this revision.Mar 5 2019, 6:26 AM

I made the changes I commented on when applying the diff. Thanks for the patch!

include/atomic
613 ↗(On Diff #189269)

typename is not needed here, this is not a dependent context.

618 ↗(On Diff #189269)

Should use a typedef instead of a using statement because we had the brilliant idea to try and support C++03 in <atomic>.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 6:50 AM
ldionne added inline comments.Mar 5 2019, 6:50 AM
include/atomic
613 ↗(On Diff #189269)

Also, you need to wrap the is_same<underlying_type<...>::type, unsigned>::value into parenthesis because this will otherwise trip up our static_assert emulation. This could have been noticed by running the test suite in c++03 mode with ./build/bin/llvm-lit -sv libcxx/test/std/atomics --param=std=c++03 (assuming a standard CMake setup).

For info: Followed-up with https://reviews.llvm.org/D58966 to fix build break on GCC.

zoecarver marked an inline comment as done.Mar 5 2019, 8:29 AM
zoecarver added inline comments.
include/atomic
613 ↗(On Diff #189269)

Thanks for fixing this! I will make sure to test C++03 next time. Is there a param to run with GCC?