Skip to content

<chrono> LWG-3536: leave duration unchanged on failed parse #2037

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
Merged
StephanTLavavej merged 5 commits into from
Aug 17, 2021
Merged

<chrono> LWG-3536: leave duration unchanged on failed parse #2037

StephanTLavavej merged 5 commits into from
Aug 17, 2021

Conversation

@yuanhongzhao
Copy link
Contributor

@yuanhongzhao yuanhongzhao commented Jul 3, 2021

Fixes #1740.

@yuanhongzhao yuanhongzhao requested a review from a team as a code owner July 3, 2021 07:06
@ghost
Copy link

ghost commented Jul 3, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@StephanTLavavej StephanTLavavej added chrono C++20 chrono LWG Library Working Group issue labels Jul 3, 2021
@StephanTLavavej StephanTLavavej self-assigned this Jul 7, 2021
mnatsuhara
mnatsuhara suggested changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Thanks for working on this issue! Apologies for the delay in review, the maintainers have been steadily working through the PR backlog. The actual product code changes look great, the only changes I'm requesting are some additional test cases! Once you add those, I'll be satisfied 😸

@StephanTLavavej StephanTLavavej mentioned this pull request Aug 11, 2021
Closed
36 tasks
mnatsuhara
mnatsuhara approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Thanks for pushing changes addressing my review comments! These changes look good to me -- moving into "Final Review" mode now 😸

StephanTLavavej
StephanTLavavej reviewed Aug 13, 2021
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this LWG issue resolution! This looks good to me - I'll push changes for minor clarity improvements.

@StephanTLavavej StephanTLavavej removed their assignment Aug 13, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 13, 2021

FYI @mnatsuhara, I pushed minor changes after you approved.

@StephanTLavavej StephanTLavavej self-assigned this Aug 14, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 14, 2021

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 3c015bd into microsoft:main Aug 17, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 17, 2021

Thanks for implementing this LWG issue resolution, and congratulations on your first microsoft/STL commit! 🎉 😻 🚀

This will ship in VS 2022 17.1 Preview 1.

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

+2 more reviewers

@MattStephanson MattStephanson MattStephanson left review comments

@mnatsuhara mnatsuhara mnatsuhara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

chrono C++20 chrono LWG Library Working Group issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LWG-3536 Should chrono::from_stream assign zero to duration for failure?

4 participants

@yuanhongzhao @StephanTLavavej @mnatsuhara @MattStephanson