Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ios_base constants should have bitmask types #3405

Merged
merged 8 commits into from Feb 23, 2023

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Feb 11, 2023

Fixes #3401

If I understand correctly we can't change types of the variables but we can add operators.

@fsb4000 fsb4000 requested a review from a team as a code owner February 11, 2023 08:05
@AlexGuteniev
Copy link
Contributor

Have you considered _BITMASK_OPS macro?

Co-authored-by: Alex Guteniev <gutenev@gmail.com>
@fsb4000
Copy link
Contributor Author

fsb4000 commented Feb 11, 2023

Thanks. I knew about the macro but I forgot about it.

@frederick-vs-ja
Copy link
Contributor

std::ios_base::openmode is still int. We should fix the type mismatch, see [ios.base.general].

@AlexGuteniev
Copy link
Contributor

std::ios_base::openmode is still int. We should fix the type mismatch, see [ios.base.general].

Not sure if it wouldn't be an ABI break

@fsb4000
Copy link
Contributor Author

fsb4000 commented Feb 11, 2023

Can we fix it?

What if a user used std::ios_base::openmode in his function. Before the type was int, and it is std::ios_base::_Openmode after.

It looks like ABI break.

I don't really understand all this about ABI break, but I remember that intmax_t was considered a very bad decision because this type is forever int64_t, because changing it to a hypothetical int128_t would be an ABI break.

@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews Feb 11, 2023
@StephanTLavavej
Copy link
Member

The variables don't seem to appear on the DLL's export surface, so it might be possible to change their types. Yes, theoretically someone could have been depending on their types in their own signatures, but it would be fairly hard (especially pre-decltype). I am pretty scared to change anything in iostreams but it might be possible, and it would be simpler and better for throughput than emitting a bunch of bitmask ops (which are unfortunately widely visible).

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Feb 11, 2023
stl/inc/xiosbase Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Feb 12, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Feb 13, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Feb 13, 2023
@StephanTLavavej
Copy link
Member

Thanks, I pushed small changes to the test.

I double-checked for changes to the release and debug DLL export surfaces, and found none. I think this is ABI-safe from a "does this affect the redist" perspective, and I think that it's unlikely to be disruptive (as I mentioned earlier, theoretically someone could have used decltype to make their own signatures depend on the types of these constants, but that is totally different from normal usage).

There is now unused enum machinery but I would prefer not to mess with that at this time (as the TRANSITION, ABI comments indicate, I once thought this affected ABI - I can't remember why).

@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Feb 15, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 22, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej changed the title _Openmode,_Seekdir,_Fmtflags,_Iostate are BitmaskTypes ios_base constants should have bitmask types Feb 23, 2023
@StephanTLavavej StephanTLavavej merged commit bd07818 into microsoft:main Feb 23, 2023
Code Reviews automation moved this from Ready To Merge to Done Feb 23, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this long-standing nonconformance! ✨ 😺 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

<ios>: std::ios_base::openmode is not a bitmask type
5 participants