Skip to content

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

Merged
StephanTLavavej merged 12 commits intomicrosoft:mainfrom
Atari2:P2467R1
Nov 11, 2022
Merged

Implement P2467R1 ios_base::noreplace: Exclusive Mode For fstreams (#2932)#3065
StephanTLavavej merged 12 commits intomicrosoft:mainfrom
Atari2:P2467R1

Conversation

@Atari2
Copy link
Contributor

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

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
@StephanTLavavej StephanTLavavej added cxx23 C++23 feature affects redist Results in changes to separately compiled bits labels Aug 31, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 31, 2022

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!

CaseyCarter
CaseyCarter approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

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

@CaseyCarter CaseyCarter removed their assignment Sep 15, 2022
StephanTLavavej
StephanTLavavej reviewed Sep 20, 2022
View reviewed changes
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 20, 2022 •
edited
Loading

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 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
Copy link
Member

StephanTLavavej commented Nov 10, 2022

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

StephanTLavavej commented Nov 11, 2022

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
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Nov 11, 2022

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

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@MattStephanson MattStephanson MattStephanson left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

affects redist Results in changes to separately compiled bits cxx23 C++23 feature

Projects

None yet

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