decouple Windows APIs from UTF16String type #15033

StefanKarpinski · 2016-02-12T01:20:17Z

This PR introduces three functions that are strictly internal to Base for now: utf8to16, utf16to8 and cwstring – which calls utf8to16 ensuring that there are no embedded NULs and add a trailing NUL. These are self-contained and allow use to interact with Windows APIs without the UTF16String type.

tkelman · 2016-02-12T01:47:23Z

base/path.jl

-            return utf8(UTF16String(buf))
-        end
+        systemerror(:longpath, n == 0)
+        x = n < length(buf)


~~why is this assigned to a variable?~~ nevermind I see how it works now - add some comments?

vtjnash · 2016-02-12T01:58:03Z

lgtm. maybe squash to 1 commit though? i'm not sure the rest are necessary

tkelman · 2016-02-12T02:00:58Z

base/env.jl

+function access_env(onError::Function, str::AbstractString)
+    var = cwstring(str)
+    len = _getenvlen(var)
+    len == 0 && return Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND ? utf8("") : onError(str)


This would be clearer as ifs

that's an impressive garden path. regardless of where i tried to start reading the expression, I had to revise my assumptions as a read more.

I may actually have screwed this up. Will fix it.

does that mean missing branch coverage?

Oh, actually no – I didn't screw it up because of the return. But yes, this is obviously confusing.

tkelman · 2016-02-12T02:08:16Z

They're logically decoupled and none are broken, so I like the current breakdown into small pieces. Couple minor places that could use some comments or rearranging for clarity, otherwise LGTM2.

StefanKarpinski · 2016-02-12T02:41:41Z

Woot! Windows build success! That's the risky part.

tkelman · 2016-02-12T02:48:48Z

Looks like we don't have any tests for clipboard. Did you play with that one manually in a VM?

StefanKarpinski · 2016-02-18T22:26:33Z

I tested clipboard manually and added tests. Technically, there's a chance that someone could copy something between clipboard(str) and clipboard() but realistically, I don't think it's an issue.

tkelman · 2016-02-19T01:24:34Z

still needs more comments - and a rebase given #15137 was merged separately

StefanKarpinski · 2016-02-19T11:19:12Z

Seems that the clipboard command doesn't work on Linux since we need xsel or xclip. Is there some way we can install one of those on Travis?

tkelman · 2016-02-19T11:53:52Z

I think so, we can add it to the big long list of addons: apt: packages:, though would still want to check that it exists before running that test so our tests can pass even in minimal environments.

decouple Windows APIs from UTF16String type

tkelman · 2016-03-05T03:03:07Z

Hm the clipboard test is failing on the mac buildbot, it actually seems to be running at a reasonable pace the last few days - https://build.julialang.org/builders/build_osx10.9-x64/builds/377/steps/shell_2/logs/stdio

Anyone have any ideas what might be causing this?

    * misc                 ERROR: LoadError: LoadError: failed process: Process(`pbcopy`, ProcessExited(1)) [1]
 [inlined code] from ./error.jl:22
 in pipeline_error(::Base.Process) at ./process.jl:623
 in open(::Base.##440#441{ASCIIString}, ::Base.CmdRedirect, ::ASCIIString) at ./process.jl:569
 [inlined code] from ./boot.jl:331
 in clipboard(::ASCIIString) at ./interactiveutil.jl:92
 [inlined code] from /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/misc.jl:384
 in anonymous at ./no file:4294967295
 in include(::ASCIIString) at ./boot.jl:264
 in include_from_node1(::ASCIIString) at ./loading.jl:417
 [inlined code] from ./util.jl:179
 in runtests(::ASCIIString) at /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/testdefs.jl:7
 in run_work_thunk(::Base.##245#246{##16#24,Tuple{ASCIIString}}, ::Bool) at ./multi.jl:714
 in remotecall_fetch(::Any, ::LocalProcess, ::ASCIIString, ::Vararg{ASCIIString}) at ./multi.jl:787
 in remotecall_fetch(::Any, ::Int64, ::ASCIIString, ::Vararg{ASCIIString}) at ./multi.jl:803
 [inlined code] from /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/runtests.jl:36
 in (::##15#23{Array{ASCIIString,1}})() at ./task.jl:431
while loading /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/misc.jl, in expression starting on line 382
 in remotecall_fetch(::Any, ::LocalProcess, ::ASCIIString, ::Vararg{ASCIIString}) at ./multi.jl:788
 in remotecall_fetch(::Any, ::Int64, ::ASCIIString, ::Vararg{ASCIIString}) at ./multi.jl:803
 [inlined code] from /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/runtests.jl:36
 in (::##15#23{Array{ASCIIString,1}})() at ./task.jl:431
 in sync_end() at ./task.jl:397
 [inlined code] from ./task.jl:406
 in (::##11#19)() at /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/runtests.jl:31
 in cd(::##11#19, ::ASCIIString) at ./file.jl:48
 in include(::ASCIIString) at ./boot.jl:264
 in include_from_node1(::UTF8String) at ./loading.jl:417
 in process_options(::Base.JLOptions) at ./client.jl:262
 in _start() at ./client.jl:318
while loading /Users/osx/buildbot/slave/build_osx10_9-x64/build/test/runtests.jl, in expression starting on line 13

StefanKarpinski · 2016-03-06T01:16:21Z

I wonder if the buildbot machines don't have usable clipboard functionality.

stevengj · 2016-04-29T15:29:07Z

base/interactiveutil.jl

@@ -152,7 +152,9 @@ end
        systemerror(:CloseClipboard, 0==ccall((:CloseClipboard, "user32"), stdcall, Cint, ()))
        plock = ccall((:GlobalLock, "kernel32"), stdcall, Ptr{UInt16}, (Ptr{UInt16},), pdata)
        systemerror(:GlobalLock, plock==C_NULL)
-        s = utf8(utf16(plock))
+        len = 0
+        while unsafe_load(plock, len+1) != 0; len += 1; end


Maybe len = ccall(:wcslen, Csize_t, (Ptr{Cwchar},), plock) would be clearer? Better yet, add a method:

@windows_only utf16to8(p::Ptr{UInt16}, len=ccall(:wcslen, Csize_t, (Ptr{UInt16},), p)) = utf16to8(pointer_to_array(p, len))

method?

stevengj · 2016-04-29T15:39:58Z

Eventually I think we'll want to export some version of these routines, for the benefit of external packages calling Windows APIs.

stevengj · 2016-04-29T15:42:30Z

base/c.jl

@@ -96,6 +94,105 @@ convert(::Type{Cstring}, s::Symbol) = Cstring(unsafe_convert(Ptr{Cchar}, s))

 # in string.jl: unsafe_convert(::Type{Cwstring}, s::WString)

+# FIXME: this should be handled by implicit conversion to Cwstring, but good luck with that


What's the obstacle to using the Cwstring conversion for ccall arguments?

I forget the details, but unsafe_convert and cconvert were not cooperative. I spent an hour or so going back and forth with @yuyichao and @vtjnash and it didn't lead to anything workable. That would be the right solution, rather than exposing these internal conversion functions.

simonbyrne · 2016-05-26T10:11:53Z

I realise I'm coming late to this party, but if this is only being used on windows, would it be better to simply use the Windows APIs, e.g. MultiByteToWideChar?

StefanKarpinski · 2016-05-26T13:30:03Z

@vtjnash suggested that at some point but I found testing the Windows API difficult. I suppose I could spin up a new VM and give it a shot. Or if someone else who has an actual Windows machine wants to take a crack at it, I'd be fine with that too.

vtjnash · 2016-05-26T13:49:34Z

I did, but by then you had already implemented it in Julia, so there wasn't really any point in switching.

StefanKarpinski · 2016-05-26T14:14:10Z

There are a few reasons to change: less code to maintain and test, less temptation for users and packages to call unexported functions that we might change or remove in the future.

tkelman added system:windows Affects only Windows unicode Related to unicode characters and encodings labels Feb 12, 2016

tkelman reviewed Feb 12, 2016
View reviewed changes

StefanKarpinski force-pushed the sk/utf16helpers branch 5 times, most recently from 793fc26 to f032a66 Compare February 18, 2016 22:14

StefanKarpinski added 2 commits February 19, 2016 06:20

sysimg.jl: load osutils.jl before c.jl

11225d1

highlander: add utf8to16, utf16to8, cwstring; excise UTF16String

103db50

StefanKarpinski force-pushed the sk/utf16helpers branch from f032a66 to f11b2ab Compare February 19, 2016 11:32

clipboard: add some basic tests (OS X & Windows)

6970fda

StefanKarpinski force-pushed the sk/utf16helpers branch from f11b2ab to 6970fda Compare February 19, 2016 11:40

StefanKarpinski added a commit that referenced this pull request Feb 19, 2016

Merge pull request #15033 from JuliaLang/sk/utf16helpers

fa4bbef

decouple Windows APIs from UTF16String type

StefanKarpinski merged commit fa4bbef into master Feb 19, 2016

StefanKarpinski deleted the sk/utf16helpers branch February 19, 2016 13:25

tkelman mentioned this pull request Mar 5, 2016

Only run clipboard test on Windows #15365

Merged

mikewl mentioned this pull request Mar 5, 2016

Failiure in misc on master #15341

Closed

StefanKarpinski mentioned this pull request Apr 28, 2016

Stringapalooza #16107

Closed

32 tasks

stevengj reviewed Apr 29, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decouple Windows APIs from UTF16String type #15033

decouple Windows APIs from UTF16String type #15033

StefanKarpinski commented Feb 12, 2016

tkelman Feb 12, 2016

vtjnash commented Feb 12, 2016

tkelman Feb 12, 2016

vtjnash Feb 12, 2016

StefanKarpinski Feb 12, 2016

tkelman Feb 12, 2016

StefanKarpinski Feb 12, 2016

tkelman commented Feb 12, 2016

StefanKarpinski commented Feb 12, 2016

tkelman commented Feb 12, 2016

StefanKarpinski commented Feb 18, 2016

tkelman commented Feb 19, 2016

StefanKarpinski commented Feb 19, 2016

tkelman commented Feb 19, 2016

tkelman commented Mar 5, 2016

StefanKarpinski commented Mar 6, 2016

stevengj Apr 29, 2016 •

edited

Loading

stevengj commented Apr 29, 2016

stevengj Apr 29, 2016

StefanKarpinski Apr 29, 2016

simonbyrne commented May 26, 2016

StefanKarpinski commented May 26, 2016

vtjnash commented May 26, 2016

StefanKarpinski commented May 26, 2016 •

edited

Loading

		@@ -96,6 +94,105 @@ convert(::Type{Cstring}, s::Symbol) = Cstring(unsafe_convert(Ptr{Cchar}, s))

		# in string.jl: unsafe_convert(::Type{Cwstring}, s::WString)

		# FIXME: this should be handled by implicit conversion to Cwstring, but good luck with that

decouple Windows APIs from UTF16String type #15033

decouple Windows APIs from UTF16String type #15033

Conversation

StefanKarpinski commented Feb 12, 2016

Choose a reason for hiding this comment

vtjnash commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Feb 12, 2016

StefanKarpinski commented Feb 12, 2016

tkelman commented Feb 12, 2016

StefanKarpinski commented Feb 18, 2016

tkelman commented Feb 19, 2016

StefanKarpinski commented Feb 19, 2016

tkelman commented Feb 19, 2016

tkelman commented Mar 5, 2016

StefanKarpinski commented Mar 6, 2016

stevengj Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

stevengj commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbyrne commented May 26, 2016

StefanKarpinski commented May 26, 2016

vtjnash commented May 26, 2016

StefanKarpinski commented May 26, 2016 • edited Loading

stevengj Apr 29, 2016 •

edited

Loading

StefanKarpinski commented May 26, 2016 •

edited

Loading