-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix some UTF-8 issues on Windows. #9812
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
Conversation
@@ -54,6 +54,12 @@ | |||
#include <assert.h> | |||
|
|||
#if defined(__WIN32__) | |||
#ifndef WIN32_LEAN_AND_MEAN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments explaining what these constants are and why they're defined?
Hm it's unfortunate that this probably means that most of our usage of windows apis are "broken" in the sense that we're not dealing with unicode strings properly. This seems like some that's tricky to get right, so I'm wary of adding this sort of functionality without an ability to prevent it regressing. Could you also add some tests with unicode filenames and such to make sure that these apis properly work? The time zone problem may be tough to test, but it would be great if we were able to get it testable as well. |
Also I'm still a bit curious as to why we need to use all the wide versions everywhere. If one still take |
Sorry, "most of If a locale related argument (like |
So just to make sure I understand this correctly, if windows hands us a |
No, it doesn't work for some files which contain non-ANSI[1] character as file name.
One more reason is
[1] ANSI is local system encoding except unicode encodings, such as ISO-8859-1, CP949, Shift_JIS. Windows has (almost) fully featured unicode functionality, but ANSI ones remain for backward compatibility. |
OK that makes sense to me now, so could you do a few things?
|
Added comments, tests, and rebased. Force push... :( |
rust_path_exists | ||
rust_path_exists_u16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this will fail to compile on osx and linux because these symbols aren't defined in rust_builtin.cpp. Feel free to just make them empty functions, that's been the pattern for platform-specific C++ helpers so far.
Nice job, and thanks again! Just two things:
|
Thanks for review! I think it's all done, but if there's something wrong, please let me know. |
I didn't consider linux; fix amended. |
|
Hm, this is using some test files in the os's tmp directory, but builders can run simultaneously on one machine, so I don't think that this is playing nicely when that's happening. Each test needs to choose a unique location to test its paths/directories. There isn't the convenience of |
… rust_localtime. This make these functions use wchar_t version of APIs, instead of char version.
Temporary path randomized; there will not be race conditions, hopefully. |
This fixes #9418 and #9618, and potential problems related to directory walking.