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

No more 'operation completed successfully' in time_zone::get_info #3122

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Sep 22, 2022

When no mapping for a Unicode character existed in the target multi-byte code page, we just returned that error from
__std_fs_convert_wide_to_narrow; instead, we additionally needed to SetLastError() after that call.

Fixes #3097 (kind of)

The issue was that if the language of the system was not representable by the current code page, we would throw GetLastError(); however, last error was never actually set.

When no mapping for a Unicode character existed in the target multi-byte
code page, we just returned that error from
`__std_fs_convert_wide_to_narrow`; instead, we additionally needed to
`SetLastError()` after that call.
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner September 22, 2022 21:24
@StephanTLavavej StephanTLavavej added this to Final Review in Code Reviews Sep 23, 2022
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 23, 2022
@strega-nil-ms strega-nil-ms added the decision needed We need to choose something before working on this label Sep 26, 2022
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Sep 28, 2022
@CaseyCarter
Copy link
Member

We discussed in the weekly maintainer meeting that this resolves the symptom, but not really the root cause of the problem. We're going to apply the PR as a clear improvement, and @strega-nil-ms will file a followup issue to investigate returning the full timezone spec instead of the localized abbreviation as a fallback when the localized abbreviation can't be transcoded from UTF-16 to the narrow encoding specified by the active code page.

@StephanTLavavej
Copy link
Member

I observe that this affects the redist, but not in a way that involves synchronized header changes, so it should be fine.

✅ No header changes => no modules impact.

@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 2022
@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 6f467fc into microsoft:main Oct 12, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 12, 2022
@StephanTLavavej
Copy link
Member

Merge completed successfully! 😹 😸 😻

@OptimumCpp
Copy link

I guess I have spotted the root cause of the problem correct behavior would be no exception. Trouble begins when Country or region and Regional format are set to seemingly incompatible values:
Problem

As a result, locale of the calendar becomes incompatible with ACP codepage, and results in transformation error. The problem can be identified and solved inside tzdb.cpp with no external side effects. It only involves the _Allocate_wide_to_narrow function. Relevant parts of the code in tzdb.cpp are lines 198~200:
relevant part0
and line 568:
relevant part1

The fix is trivial; just change the former to:
fix part0
and the later to:
fix part1

The signature of _Allocate_narrow_to_wide can also be modified for conformance and future use. But that currently does not add any value to code. Will you please ship a patch now? Or I should request a pull on code repository?
A crash due to regional settings is really frustrating; TBH it is actually a bug, and now it seems to have been spotted. So, why not remove it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working decision needed We need to choose something before working on this
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

std::chrono::time_zone::get_info throws an exception saying: The operation completed successfully
5 participants