Skip to content

Implement P3223R2 Making istream::ignore() Less Surprising#5604

Merged
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
frederick-vs-ja:p3223r2
Aug 8, 2025
Merged

Implement P3223R2 Making istream::ignore() Less Surprising#5604
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
frederick-vs-ja:p3223r2

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Jun 22, 2025 •
edited
Loading

As a Defect Report against old standard modes. Fixes #5618.

As a result of DR-backing P3223R2, the new overload is templated, and thus this PR happens to "constrain the new overload to prevent ambiguities" and keeps in.ignore(100, -1L) well-formed.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner June 22, 2025 14:32
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews Jun 22, 2025
@frederick-vs-ja frederick-vs-ja changed the title Implement P3223R2 Making std::istream::ignore Less Surprising Implement P3223R2 Making std::istream::ignore() Less Surprising Jun 23, 2025
@frederick-vs-ja frederick-vs-ja changed the title Implement P3223R2 Making std::istream::ignore() Less Surprising Implement P3223R2 Making istream::ignore() Less Surprising Jun 23, 2025
@github-project-automation github-project-automation bot moved this from Initial Review to Done in STL Code Reviews Jun 23, 2025
@github-project-automation github-project-automation bot moved this from Done to Initial Review in STL Code Reviews Jun 23, 2025
@StephanTLavavej StephanTLavavej added the cxx26 C++26 feature label Jun 24, 2025
@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2025
@Zingam
Copy link
Contributor

Zingam commented Jul 7, 2025

@frederick-vs-ja (This is not a review. Just a couple of questions.) Was the paper voted in as DR? I guess not. My reading of the Standard is that resolving the ambiguity is not required is that so?
I made an attempt to implement the paper in libc++ but Windows is giving me troubles llvm/llvm-project#147007

@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Jul 8, 2025 •
edited
Loading

Was the paper voted in as DR? I guess not.

I guess not, either. But according to the paper issue I think it should be treated as a DR.

My reading of the Standard is that resolving the ambiguity is not required is that so?

Yes. In this PR the ambiguity is resolved "by accident", which is a consequence of backporting. I don't think the ambiguity resolution is disallowed, either.

@StephanTLavavej StephanTLavavej removed their assignment Aug 6, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews Aug 6, 2025
@StephanTLavavej StephanTLavavej mentioned this pull request Aug 4, 2025
Open
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews Aug 7, 2025
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 7, 2025

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

@StephanTLavavej StephanTLavavej merged commit fae8c55 into microsoft:main Aug 8, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews Aug 8, 2025
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 8, 2025

🐱 😸 🐈

@frederick-vs-ja frederick-vs-ja deleted the p3223r2 branch August 8, 2025 17:47
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

Assignees

No one assigned

Labels

cxx26 C++26 feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

P3223R2 Making istream::ignore() Less Surprising

3 participants

@frederick-vs-ja @Zingam @StephanTLavavej