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.

Sign up for GitHub

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

Jump to bottom

Implement P2467R1 ios_base::noreplace: Exclusive Mode For fstreams (#2932) #3065

Merged
merged 12 commits into from Nov 11, 2022
Merged

Implement P2467R1 ios_base::noreplace: Exclusive Mode For fstreams (#2932) #3065

merged 12 commits into from Nov 11, 2022

Conversation

Atari2
Copy link
Contributor

@Atari2 Atari2 commented Aug 31, 2022 •
edited by StephanTLavavej

As was pointed out by @StephanTLavavej, since this PR has changes to fiopen.cpp, it will be affects redist.
While writing the tests I tried to avoid having to change things that could affect binary compatibility but it seemed like the only option was to add the now-valid entries to the array in _Xfiopen, otherwise the calls to it would just fail.

Fixes #2932.

@Atari2 Atari2 requested a review from a team as a code owner August 31, 2022 08:59
MattStephanson
MattStephanson reviewed Aug 31, 2022
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added cxx23 C++23 feature affects redist Results in changes to separately compiled bits labels Aug 31, 2022
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation Aug 31, 2022
@StephanTLavavej
Copy link
Member

Thanks! I updated your PR description to say "Fixes <issue number>", which tells GitHub to link the tracking issue and close it when this PR is merged.

@Atari2
Fix macro guard usage and don't mask out ios_base::_Noreplace from …
cb4420b 
…`mode`

Remove macro guards from .cpp files and use _HAS_CXX23 for the .h files.
Additionally, as @MattStephanson pointed out, since _fsopen already handles the 'x' modifiers correctly, the masking out and additional check of ios_base::_Noreplace has been removed in favour of just passing it directly to _Xfsopen and adding strings for the modes there. This also avoids the "test open" to check if the file already exists which avoids a TOCTOU issue.
strega-nil-ms
strega-nil-ms approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@strega-nil-ms strega-nil-ms moved this from Initial Review to Final Review in Code Reviews Sep 14, 2022
CaseyCarter
CaseyCarter approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just style nits with the test, for which I'll push a cleanup.

tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Sep 15, 2022
@CaseyCarter CaseyCarter removed their assignment Sep 15, 2022
StephanTLavavej
StephanTLavavej reviewed Sep 20, 2022
View reviewed changes
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
stl/src/fiopen.cpp Show resolved Hide resolved
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Show resolved Hide resolved
tests/std/tests/P2467R1_exclusive_mode_fstreams/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 20, 2022 •
edited

Thanks @Atari2, this looks great! I've pushed a merge with main in order to resolve trivial adjacent-add conflicts with the Modules PR that just landed (in yvals_core.h and tests/std/test.lst, as the paper numbers and macro names sorted close together), followed by fine-grained commits to address minor things I noticed. (The most significant being a pre-emptive fix for the /clr:pure internal build.) Also FYI @CaseyCarter @strega-nil-ms as I pushed after you approved.

While some affects redist PRs can be merged for locked-redist releases (when the redist's behavior changes, but we're comfortable with the compatibility implications), this feature requires an updated redist or it'll fail at runtime, so we'll place this PR in cryo-stasis (blocked status) until changes start flowing into VS 2022 17.6. I believe that this will happen at the end of October (note: this has no relationship to public release dates of Previews - it is purely when internal merges switch over from one release branch to another).

✅ Also, no changes are needed for modules, as this simply adds a static constexpr data member.

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Blocked in Code Reviews Sep 20, 2022
@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label Sep 20, 2022
@StephanTLavavej StephanTLavavej removed the blocked Something is preventing work on this label Nov 10, 2022
@StephanTLavavej StephanTLavavej moved this from Blocked to Ready To Merge in Code Reviews Nov 10, 2022
@StephanTLavavej
Copy link
Member

VS 2022 17.6 Preview 1's redist will be unlocked, so we should be able to mirror this now. I've pushed a conflict-free merge with main to pick up the ~2 months of changes there (and submodule updates), to be careful.

@StephanTLavavej StephanTLavavej self-assigned this Nov 11, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit c550bd0 into microsoft:main Nov 11, 2022
Code Reviews automation moved this from Ready To Merge to Done Nov 11, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this feature in a binary-compatible way! 😻 🚀 🎉

@Atari2 Atari2 deleted the P2467R1 branch November 12, 2022 10:15
@frederick-vs-ja frederick-vs-ja mentioned this pull request Mar 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattStephanson MattStephanson MattStephanson left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
affects redist Results in changes to separately compiled bits cxx23 C++23 feature
Projects
No open projects
Code Reviews
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P2467R1 ios_base::noreplace: Exclusive Mode For fstreams
5 participants
@Atari2 @StephanTLavavej @CaseyCarter @MattStephanson @strega-nil-ms