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

LWG-3554 Add const charT* overloads for chrono::parse #2261

Merged
merged 6 commits into from
Nov 3, 2021

Conversation

cpplearner
Copy link
Contributor

Implements LWG-3554 "chrono::parse needs const charT* and basic_string_view<charT> overloads" (despite the title, LWG-3554 does not add basic_string_view overloads).

Fixes #1928

@cpplearner cpplearner requested a review from a team as a code owner October 6, 2021 08:30
stl/inc/chrono Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Oct 6, 2021
stl/inc/chrono Outdated Show resolved Hide resolved
basic_string<_CharT, _Traits, _Alloc>* _Abbrev;
minutes* _Offset;
};

template <class _CharT, class _Traits, class _Alloc, class _Parsable>
struct _Time_parse_iomanip {
Copy link
Contributor

@MattStephanson MattStephanson Oct 7, 2021

Choose a reason for hiding this comment

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

Maybe a // TRANSITION, ABI comment to the effect that this can be replaced with _Time_parse_iomanip_c_str in the future?

Edit: this is showing more context than I intended. I was referring specifically to the struct _Time_parse_iomanip { line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The small code duplication is not bad enough to worry me, personally. Storing a reference to basic_string also has a tiny benefit, that is the format string is not invalidated if the basic_string is resized after the call to parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the manipulator is supposed to live long enought for the format string to be resized, but fair enough regarding the amount of code duplication.

@StephanTLavavej StephanTLavavej added the chrono C++20 chrono label Oct 13, 2021
@CaseyCarter CaseyCarter self-assigned this Oct 20, 2021
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Oct 28, 2021
@StephanTLavavej StephanTLavavej self-assigned this Nov 2, 2021
@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 75d373a into microsoft:main Nov 3, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing this LWG issue resolution in a timely manner! 😹 ⌚ 🎉

@cpplearner cpplearner deleted the chrono-parse branch November 3, 2021 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono LWG Library Working Group issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LWG-3554 chrono::parse needs const charT* and basic_string_view<charT> overloads
4 participants