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.

Sign up for GitHub

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

Jump to bottom

wcsftime instead of strftime for different locales (#27239) #27273

Merged
merged 1 commit into from
Jul 3, 2018
Merged

wcsftime instead of strftime for different locales (#27239) #27273

merged 1 commit into from
Jul 3, 2018

Conversation

alkorang
Copy link
Contributor

Fix #27239

The problem was invalid UTF-8 characters from strftime() depending on Windows system locales.
Solution is using wcsftime() to get UTF-16 and transcode() it into UTF-8 on Windows.

Keno
Keno reviewed May 27, 2018
View reviewed changes
base/libc.jl Outdated
if Sys.iswindows()
function wcsftime(fmt::AbstractString, tm::TmStruct)
wctimestr = Vector{Cwchar_t}(undef, 128)
n = ccall(:wcsftime, Csize_t, (Ptr{Cwchar_t}, Csize_t, Cwstring, Ref{Libc.TmStruct}),
Copy link
Member

Choose a reason for hiding this comment

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

Drop the Libc.?

@kshyatt kshyatt added the system:windows Affects only Windows label May 27, 2018
@Keno
Copy link
Member

Keno commented May 27, 2018

Add a test?

ararslan
ararslan reviewed May 27, 2018
View reviewed changes
base/libc.jl Outdated
n == 0 && return ""
return transcode(String, wctimestr[1:n])
end
strftime(fmt::AbstractString, tm::TmStruct) = wcsftime(fmt, tm)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than defining both strftime and wcsftime, you could just call the function strftime. Currently you're effectively defining two identical functions on Windows.

@ararslan ararslan added the needs tests Unit tests are required for this change label May 27, 2018
@alkorang
Copy link
Contributor Author

alkorang commented May 27, 2018 •
edited
Loading

@Keno @ararslan
Thank you for your reviews!

Add a test?

Where would be the best to put tests? Is it okay to put them in test/misc.jl?

@ararslan
Copy link
Member

Yep, I think test/misc.jl would be a fine place for it. 🙂

@StefanKarpinski
Copy link
Member

Commentary: https://discourse.julialang.org/t/strftime-strptime-bug-27239-is-present-on-all-platforms-not-just-windows/11191.

@alkorang alkorang changed the title On Windows use wcsftime instead of strftime to support different locales (#27239) wcsftime/wcsptime instead of strftime/strptime for different locales (#27239) May 28, 2018
@alkorang
Copy link
Contributor Author

alkorang commented May 28, 2018 •
edited
Loading

Commentary: https://discourse.julialang.org/t/strftime-strptime-bug-27239-is-present-on-all-platforms-not-just-windows/11191.

The comment from @ScottPJones is that this can happen in any system with different locales other than UTF-8, For example on the Mac,

julia> setlocale(lc) = unsafe_string(ccall((:setlocale,"libc"), Cstring,(Cint,Cstring),0,lc))
setlocale (generic function with 1 method)
julia> setlocale("ko_KR.UTF-8")
"ko_KR.UTF-8"

julia> Libc.strftime("%Y-%m-%d %A %H:%M:%S %Z", time())
"2018-05-28 월요일 02:46:27 EDT"

julia> setlocale("ko_KR.CP949")
"ko_KR.CP949"

julia> Libc.strftime("%Y-%m-%d %A %H:%M:%S %Z", time())
"2018-05-28 \xbf\xf9\xbf\xe4\xc0\xcf 02:46:36 EDT"

So I changed the title.

@kshyatt Could you remove the windows tag, please?

@KristofferC KristofferC removed the system:windows Affects only Windows label May 28, 2018
@alkorang alkorang changed the title wcsftime/wcsptime instead of strftime/strptime for different locales (#27239) wcsftime instead of strftime for different locales (#27239) Jun 2, 2018
@alkorang
Copy link
Contributor Author

alkorang commented Jun 2, 2018 •
edited
Loading

Currently wcsptime is not included in Julia. Change the title to fix strftime first.

julia> r = ccall(:wcsptime, Cwstring, (Cwstring, Cwstring, Ref{TmStruct}), timestr, fmt, tm)
ERROR: ccall: could not find function wcsptime
Stacktrace:
 [1] top-level scope at .\<missing>:0

Edit:
To make it clear, wcsptime is not in C standard library, neither time.h nor wchar.h.
wcsftime is in time.h so we can call it from libc.

@alkorang
Copy link
Contributor Author

alkorang commented Jun 2, 2018

Yep, I think test/misc.jl would be a fine place for it. 🙂

I added tests in test/misc.jl.

@alkorang alkorang closed this Jun 18, 2018
@ararslan
Copy link
Member

Did I miss something? Why was this closed, @alkorang?

@alkorang
Copy link
Contributor Author

@ararslan
Nothing wrong. I pressed the close button by mistake....

@alkorang alkorang reopened this Jun 18, 2018
@ararslan
Copy link
Member

Happens to the best of us!

@alkorang
Copy link
Contributor Author

It finally passed all checks after more rebasing!
@Keno @ararslan
Could you review the tests and merge this pr?

@KristofferC KristofferC removed the needs tests Unit tests are required for this change label Jun 26, 2018
@StefanKarpinski StefanKarpinski mentioned this pull request Jun 28, 2018
Closed
@m-j-w
Copy link
Contributor

m-j-w commented Jul 3, 2018

Any chance this gets merged soon? Currently 0.7 is not really usable on Win 10 with non-english locale. Thanks!

@KristofferC
Copy link
Member

Let's rerun tests and merge when CI is green.

@KristofferC KristofferC closed this Jul 3, 2018
@KristofferC KristofferC reopened this Jul 3, 2018
@ararslan
Copy link
Member

ararslan commented Jul 3, 2018

It might be worthwhile to add a NEWS entry under "library improvements" or something, since this will be of interest for folks in other locales.

@KristofferC KristofferC merged commit 60174a9 into JuliaLang:master Jul 3, 2018
@vtjnash vtjnash mentioned this pull request Jun 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alkorang @Keno @ararslan @StefanKarpinski @m-j-w @KristofferC @kshyatt