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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: include/atomic:603
+
+#endif
 
----------------
ldionne wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > jfb wrote:
> > > > EricWF wrote:
> > > > > jwakely wrote:
> > > > > > zoecarver wrote:
> > > > > > > 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). 
> > > > > > 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.
> > > > > 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. 
> > > > > 
> > > > 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.
> > > 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. 
> > I guess the issue with that is things which are defined as `memory_order` still couldn't accept ints. 
> > 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.
I will keep it the same then?

Side question, what does DR stand for? 


================
Comment at: include/atomic:617
+
+using memory_order_t = _VSTD::underlying_type<memory_order>::type; // unsigned
+
----------------
ldionne wrote:
> 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.
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. 


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

https://reviews.llvm.org/D58201