Skip to content

<xloctime>: time_get::get can still assert 'istreambuf_iterator is not dereferenceable' when the format is longer than the stream - #2851

Merged
Stephan T. Lavavej (StephanTLavavej) merged 7 commits into
microsoft:mainfrom
Alikhalesi:stl_gettime_issue
Jul 14, 2022
Merged

Conversation

@Alikhalesi

@Alikhalesi Alikhalesi commented Jul 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves #2848

@Alikhalesi
Alikhalesi requested a review from a team as a code owner July 9, 2022 11:08
@ghost

Deleted user (ghost) commented Jul 9, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@fsb4000

This comment was marked as resolved.

Sign in to view
fsb4000
Igor Zhukov (fsb4000) reviewed Jul 9, 2022
View reviewed changes

@fsb4000 Igor Zhukov (fsb4000) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks again for your hard work. I left some comments about the code style.

Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp
Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated
Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated
fsb4000
Igor Zhukov (fsb4000) reviewed Jul 9, 2022
View reviewed changes
Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the bug Something isn't working label Jul 9, 2022
@Alikhalesi
add test for wchar_t and edit code style
b8fea35
@Alikhalesi
Merge branch 'stl_gettime_issue' of https://github.com/Alikhalesi/STL …
e4bc546 
…into stl_gettime_issue

# Conflicts:
#	tests/std/tests/Dev11_0836436_get_time/test.cpp
@Alikhalesi
edit format
0d09f1d
MattStephanson
Matt Stephanson (MattStephanson) reviewed Jul 10, 2022
View reviewed changes

@MattStephanson Matt Stephanson (MattStephanson) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you're interested, this might be a good opportunity to harmonize the loops here in _Getfmt and in get. I believe the latter is closer to what the Standard requires in [locale.time.get.members]/8. _Getfmt has some additional assumptions for performance purposes, but I think the overall logic should be the same, so it might be more clear and maintainable if the two loops are doing comparable tests in the same order.

If you don't want to go that route, though, I still think it would be better to have _First == _Last as a standalone test at the beginning of the loop, for performance reasons. IOW, if (*_Fmtfirst == '%') would become an else if following the _First == _Last test. Otherwise, we could make several calls to do_get even after the stream is eof.

Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated
@Alikhalesi

Alikhalesi commented Jul 10, 2022

Copy link
Copy Markdown
Contributor Author

If you're interested, this might be a good opportunity to harmonize the loops here in _Getfmt and in get. I believe the latter is closer to what the Standard requires in [locale.time.get.members]/8. _Getfmt has some additional assumptions for performance purposes, but I think the overall logic should be the same, so it might be more clear and maintainable if the two loops are doing comparable tests in the same order.

If you don't want to go that route, though, I still think it would be better to have _First == _Last as a standalone test at the beginning of the loop, for performance reasons. IOW, if (*_Fmtfirst == '%') would become an else if following the _First == _Last test. Otherwise, we could make several calls to do_get even after the stream is eof.

Would you suggest a good comment for this standalone test?

fsb4000
Igor Zhukov (fsb4000) reviewed Jul 10, 2022
View reviewed changes
Comment thread tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated
@fsb4000 Igor Zhukov (fsb4000) mentioned this pull request Jul 10, 2022
Merged
MattStephanson
Matt Stephanson (MattStephanson) reviewed Jul 10, 2022
View reviewed changes
Comment thread stl/inc/xloctime
@MattStephanson

Matt Stephanson (MattStephanson) commented Jul 10, 2022

Copy link
Copy Markdown
Contributor

Would you suggest a good comment for this standalone test?

This isn't a test as in "unit test", so no comment needed, in my opinion.

MattStephanson
Matt Stephanson (MattStephanson) approved these changes Jul 11, 2022
View reviewed changes

@MattStephanson Matt Stephanson (MattStephanson) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jul 12, 2022

Copy link
Copy Markdown
Member

Looks good to me too. I agree that testing _First == _Last at the beginning of each loop iteration follows [locale.time.get.members]/8 - in particular, that this happens before the whitespace-skipping logic.

strega-nil-ms
nicole mazzuca (strega-nil-ms) approved these changes Jul 12, 2022
View reviewed changes

@strega-nil-ms nicole mazzuca (strega-nil-ms) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems clearly correct according to the standardese.

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jul 13, 2022

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 16207c8 into microsoft:main Jul 14, 2022
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jul 14, 2022

Copy link
Copy Markdown
Member

Thanks for fixing this runtime correctness bug, and congratulations on your second microsoft/STL commit! 🎉 😻 🚀

This will ship in VS 2022 17.4 Preview 2.

@Alikhalesi
Alikhalesi deleted the stl_gettime_issue branch July 15, 2022 00:39
Igor Zhukov (fsb4000) pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@Alikhalesi @fsb4000
<xloctime>: time_get::get can still assert 'istreambuf_iterator is no…
c70bd98 
…t dereferenceable' when the format is longer than the stream (microsoft#2851)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+3 more reviewers
@fsb4000 Igor Zhukov (fsb4000) fsb4000 approved these changes
@MattStephanson Matt Stephanson (MattStephanson) MattStephanson approved these changes
@strega-nil-ms nicole mazzuca (strega-nil-ms) strega-nil-ms approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej)

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<xloctime>: time_get::get can still assert 'istreambuf_iterator is not dereferenceable' when the format is longer than the stream

5 participants

@Alikhalesi @fsb4000 @MattStephanson @StephanTLavavej @strega-nil-ms