Skip to content

Conversation

@MattStephanson
Copy link
Contributor

@MattStephanson MattStephanson commented Feb 4, 2021

Fixes #1606. Also:

  • failbit should be set if the stream is EOF while not parsing a specifier.
  • A '%' at the end of a format string is an error, not a literal.
  • Reads leading spaces, in addition to leading zeros.
  • Consistently allow 60 for seconds field.

Also:
 - failbit should be set if the stream is EOF while not parsing a specifier.
 - A '%' at the end of a format string is an error, not a literal.
 - Reads leading spaces, in addition to leading zeros.
@MattStephanson MattStephanson requested a review from a team as a code owner February 4, 2021 02:08
@MattStephanson
Copy link
Contributor Author

In #1606 (comment), @StephanTLavavej noted

Separately, I am surprised that we don't failbit when we were unable to parse all fields, but I am not an iostreams expert - this part may be intentional.

This appears to be the product of a perhaps-unintended interaction between [locale.time.get.members]/8 and [locale.time.get.virtuals]/12. The later requires only eofbit to be set if the stream ends while parsing a specifier in time_get::do_get. Afterwards, back in time_get::get, the test for err == ios_­base​::​goodbit is before the test for s == end, and the loop exits without setting any additional flags. So if the EOF happens in do_get, only eofbit is set. In contrast, if EOF occurs in get while parsing a literal or whitespace, then failbit is also set. I'm not a fan of this inconsistency. Maybe an LWG Issue would be worthwhile?

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Feb 4, 2021
more consistency with existing test code style
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@MattStephanson

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@MattStephanson MattStephanson marked this pull request as draft February 7, 2021 07:28
@MattStephanson MattStephanson marked this pull request as ready for review February 7, 2021 07:29
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej self-assigned this Feb 24, 2021
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! I think I'm following all of the logic here (I especially like how it mirrors the Standardese more closely now), and the added test coverage is great (along with the fact that none of the existing tests are affected). I was the most curious about counting spaces as digits, but that appears to handle the final test case and the strptime wording. I'll push a couple of commits for WP citations and consistent case-insensitive comparisons, then I think this is ready for final review.

@StephanTLavavej StephanTLavavej removed their assignment Mar 2, 2021
@StephanTLavavej StephanTLavavej merged commit 3c5e7da into microsoft:main Mar 2, 2021
@StephanTLavavej
Copy link
Member

Thanks for fixing these bugs - parsing times without delimiters has been a top customer request! 🎉 📅 ⌚ 😺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<iomanip>: get_time handles leading zeros incorrectly

3 participants