Skip to content

Convert timezone to utf-8 on Windows #10281

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

Merged
merged 2 commits into from
Nov 8, 2013
Merged

Conversation

klutzy
Copy link
Contributor

@klutzy klutzy commented Nov 5, 2013

Previously #9418 fixed utf-8 assertion issue by wcsftime,
but the function didn't work as expected: it follows the locale
set by setlocale(), not the system code page.
This patch fixes it by manual multibyte-to-unicode conversion.

Previously rust-lang#9418 fixed utf-8 assertion issue by wcsftime,
but the function didn't work as expected: it follows the locale
set by setlocale(), not the system code page.
This patch fixes it by manual multibyte-to-unicode conversion.
@alexcrichton
Copy link
Member

Hm, I was hoping that the last time around we would have some tests to exercise this functionality, but it appears that they weren't added. Is it possible to execute the unicode paths in tests?

@klutzy
Copy link
Contributor Author

klutzy commented Nov 5, 2013

The actual problem is that wcsftime thinks input is latin1 regardless of system locale. (wow!)
So at least we don't get any is_utf8() assertion failure as before, but instead we get mojibake unicode string.
For example, in my machine, extra::time::now().tm_zone is ~"\xb4\xeb\xc7\xd1\xb9\xce\xb1\xb9 \xc7\xa5\xc1\xd8\xbd\xc3" (which is in bytes [0xC2, 0xB4, 0xC3, 0xAB, 0xC3, 0x87, 0xC3, ...)

Also, I'm going to disable extra::time test on Windows: the test uses std::os::setenv("TZ", "America/Los_Angeles"); but Windows does not change timezone at all.

@klutzy
Copy link
Contributor Author

klutzy commented Nov 8, 2013

Second patch is less related to pr title :-/ but anyway this correctly sets local time zone.

// `SetEnvironmentVariable`, which `os::setenv` internally uses.
// It is why we use `putenv` here.
extern {
#[link_name = "_putenv"]
Copy link
Member

Choose a reason for hiding this comment

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

Does this compile on linux and osx? I'm a little surprised that this needs the link_name attribute. Why not just call the function fn _putenv?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this probably relies on our removal of obviously dead branches in the frontend, which might not be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I forgot #[cfg] and cfg! are different thing. This won't compile on non-windows.
#[link_name = "_putenv"] is an artifact (I'll remove it!) from cancelled plan of using putenv on other platforms.

bors added a commit that referenced this pull request Nov 8, 2013
Previously #9418 fixed utf-8 assertion issue by wcsftime,
but the function didn't work as expected: it follows the locale
set by setlocale(), not the system code page.
This patch fixes it by manual multibyte-to-unicode conversion.
@bors bors closed this Nov 8, 2013
@bors bors merged commit fe18fe0 into rust-lang:master Nov 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants