restore non-copying String behavior, add unsafe_string*, ... #16731

stevengj · 2016-06-03T02:32:56Z

This PR closes #16470 and #16713. It:

Restores the old non-copying behavior of the String(a::Vector{UInt8}) constructor, which once again takes ownership of the array. bytestring(a) is now deprecated to String(copy(a)).
Renames String(ptr, len) to unsafe_string(ptr, len) (this function makes a copy from a pointer).
Renames pointer_to_string(ptr, len) to ~~unsafe_string_wrapper(ptr, len)~~ unsafe_wrap(String, ptr, len), which is hopefully more descriptive and now begins with unsafe as people seem to prefer. (This function does not copy the date, but simply "wraps" a String around the data.)
Renames pointer_to_array to ~~unsafe_array_wrapper~~ unsafe_wrap(Array, ...) for consistency.

~~(Note that I had to work around #16730 to document the String(a::Vector{UInt8}) constructor by calling doc! manually.)~~

In the course of making this change, I had to manually inspect every call to String(...), and I cleaned up many of the call sites somewhat.

stevengj · 2016-06-03T02:37:11Z

Another possible name, based on @nalimilan's suggestion, would be unsafe_wrapper(String, ptr, len) and unsafe_wrapper(Array, ptr, len). I don't have strong feelings on this.

tkelman · 2016-06-03T03:22:00Z

If we're going to be renaming both these things, +1 to using a more general unsafe_wrapper(T, ptr, len) which would be extensible.

vtjnash · 2016-06-03T05:22:06Z

base/libc.jl

@@ -159,7 +159,7 @@ function strftime(fmt::AbstractString, tm::TmStruct)
    if n == 0
        return ""
    end
-    return String(pointer(timestr), n)
+    return unsafe_string(pointer(timestr), n)


I think String(timestr[1:n]) would be better here

Looks like the same pattern is used in several other places. Should they also be changed?

Probably. But there are also a lot of unsafe_string(pointer(buffer)) usages that are not so easily changed, because they use nul-termination to compute the string length.

vtjnash · 2016-06-03T05:36:29Z

The unsafe_wrap(String, ptr, len) suggestion sounds good to me. The rest of the PR lgtm.

tkelman · 2016-06-03T05:49:44Z

base/client.jl

        end
        # load file
        if !isempty(ARGS) && !isempty(ARGS[1])
            # program
            repl = false
            # remove filename from ARGS
-            global PROGRAM_FILE = String(shift!(ARGS))
+            global PROGRAM_FILE = shift!(ARGS)


maybe a typeassert just in case?

ARGS is a Vector{String}, so a typeassert seems superfluous. The only reason for the String call here was that this used to be UTF8String(shift!(ARGS)) and in those days the conversion was needed to avoid having the type of PROGRAM_FILE depend on the arguments. I think it just got auto search-and-replaced by String when @StefanKarpinski made the switch.

nalimilan · 2016-06-03T08:13:39Z

+1 for the cleanup!

I wonder about unsafe_string though, since it has no parallel for arrays. Maybe we should even merge it into unsafe_wrapper via a copy argument? It would be more natural to me, since the two operations are very similar.

nalimilan · 2016-06-03T08:24:34Z

base/pointer.jl

+This function is labelled "unsafe" because it will crash if `pointer` is not
+a valid memory address to data of the requested length.
+"""
+unsafe_array_wrapper


Cosmetic point: maybe more logical to document function unsafe_array_wrapper end before its methods definitions?

You can't document it before it is defined.

you can define it with the zero-method function end syntax though, just for the purposes of documenting

Ah, I see. (It would be good if this were explained in the manual. There's an obscure comment about the function end syntax being "preferred" with no explanation.)

yes it would. we should write down a list of new things that are poorly documented that you've been noticing.

stevengj · 2016-06-03T17:34:10Z

Renamed to unsafe_wrap(T, ptr, ...) for T=Array or T=String.

stevengj · 2016-06-03T17:56:10Z

base/datafmt.jl

-    open(fname) do io
-        Mmap.mmap(io, Vector{UInt8}, (Int(fsz),))
+    fsz = filesize(input)
+    if use_mmap && fsz > 0 && fsz < typemax(Int)


This check is from the original code, but I think the fsz > 0 check is probably because of #10516, so this check can be removed now.

And I don't understand the typemax(Int) check.... is there any circumstance in which fsz > typemax(Int) and readstring(input) will succeed? And why would mmap fail if fsz == typemax(Int)?

@tanmaykm, you added the original version of this code in #3468, can you comment?

Hmm, or actually it was @timholy in dcb01ae, for #3658. Is this still relevant?

The current mmap actually requires len < typemax(Int) - PAGESIZE

And why don't we use mmap by default on Windows? ... ah, that is #4664. In general, this code probably needs revisiting.

many people want to see this file removed from base entirely

Sometimes I think this drive to push things out of Base is venturing too much into purism for its own sake. Julia's core market is technical users, and the "batteries included" philosophy is very appealing there. Also, because Julia is so rapidly evolving, it is a struggle to keep packages up to date, whereas including key functionality in Base makes it much easier to update it along with changes to the core language. (It also makes it easier to evaluate changes, because we immediately see their impact.)

But in any case, not a debate for this PR.

we constantly get complaints that readcsv is slow. independent developments outside of base are already better. don't want to get into Python's situation of broken batteries that no one uses because the packages are better and the stdlib is frozen. need to limit scope if julia the language is going to get to 1.0 soon enough for it to succeed outside of academic early adopters.

things being developed in different repos is just a workflow and automation problem that is easily solvable.

StefanKarpinski · 2016-06-03T19:44:58Z

Thank you, @stevengj! This was a wonderful birthday present. 🎂 🎉 🎈

stevengj · 2016-06-04T15:43:26Z

I can't reproduce the Travis method ambiguity error on OSX, and it's weird that this failure doesn't occur on any other platform. Maybe I should just rebase and try again?

stevengj · 2016-06-05T23:25:59Z

Yay, tests are green.

TotalVerb · 2016-06-06T01:11:38Z

Was the original ambiguity problem a Julia bug? That should probably be reported as a separate issue.

stevengj · 2016-06-06T01:42:44Z

I'm not actually sure, since I couldn't reproduce it. I could see how the original method definitions might be ambiguous, though I didn't think about it too hard since the code needed simplifying anyway.

TotalVerb · 2016-06-06T01:47:36Z

I don't understand how this is supposed to work. Is this expected?

julia> f{T}(::Union{Type{Array},Type{Array{T}}}, ::Integer) = 1
f (generic function with 1 method)

julia> f(Array, 1)
ERROR: MethodError: no method matching f(::Type{Array}, ::Int64)
Closest candidates are:
  f{T}(::Union{Type{Array{T,N}},Type{Array}}, ::Integer)
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

julia> f{T}(::Union{Type{Array},Type{Array{T}}}, ::Int) = 2
f (generic function with 2 methods)

julia> f(Array, 1)
ERROR: MethodError: f(::Type{Array}, ::Int64) is ambiguous. Candidates:
  svec(Tuple{#f,Type{Array},Int64},svec(T),f{T}(::Union{Type{Array{T,N<:Any}},Type{Array}}, ::Int64) at REPL[4]:1)
  svec(Tuple{#f,Type{Array},Int64},svec(T),f{T}(::Union{Type{Array{T,N<:Any}},Type{Array}}, ::Integer) at REPL[2]:1)
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

I think what I don't understand here is:

why the first method didn't match,
and how there can be an ambiguity when the first method didn't match in the first place.

If I'm not missing something obvious here, I'll report it as a separate issue.

yuyichao · 2016-06-06T01:50:46Z

The type parameter cannot be determined.

yuyichao · 2016-06-06T01:51:24Z

Ref #13702

TotalVerb · 2016-06-06T02:07:07Z

Got it. Though the issue referenced there, #3738, has been closed. I suppose that was a special case, and that case has been fixed?

If these things don't work, maybe this line:

function unsafe_wrap{T,N}(::Union{Type{Array},Type{Array{T}},Type{Array{T,N}}}
                          p::Ptr{T}, dims::NTuple{N,Int}, own::Bool=false)

should be changed also? Or does the presence of T in Ptr{T} resolve the issue?

yuyichao · 2016-06-06T02:13:20Z

This is at least possible to handle and it seems that this is correctly handled on master (which is why #3738 is closed). The case you had above should not work and explaining it better in the doc is why #13702 is still open AFAICT.

tkelman · 2016-06-07T11:25:25Z

base/strings/basic.jl

-Create a string from the address of a C (0-terminated) string encoded as UTF-8.
-A copy is made so the pointer can be safely freed. If `length` is specified, the
-string does not have to be 0-terminated.
+See also [`unsafe_string_wrapper`](:func:`unsafe_string_wrapper`), which takes a pointer


not called that any more

tkelman · 2016-06-07T11:28:12Z

base/pointer.jl

+"""
+    unsafe_wrap(Array, pointer, dims, own=false)
+
+Wrap a native pointer as a Julia `Array `object. The pointer element type determines the array


``Array object.

JeffBezanson · 2016-06-08T18:59:50Z

A big +1 to merging this.

…unsafe_string_wrapper, and unsafe_array_wrapper; restore non-copying behavior of String(::Vector{UInt8}) constructor (closes JuliaLang#16470, closes JuliaLang#16713)

…opefully remove occasional method ambiguity errors: overflow is already checked by convert, and sizes ≤ 0 are already checked in jl_ptr_to_array

stevengj · 2016-06-08T19:47:21Z

Rebased.

StefanKarpinski · 2016-06-08T22:41:34Z

I'm guessing that the libgit2 error is unrelated.

tkelman · 2016-06-08T22:43:13Z

#16555. merge away

bkamins · 2019-06-06T12:07:55Z

@stevengj This PR in particular introduced https://github.com/JuliaLang/julia/blob/master/stdlib/DelimitedFiles/src/DelimitedFiles.jl#L229

This makes readdlm mutate input if it is Vector{UInt8}. Was it intended or rather String(copy(input)) should be a proper implementation here?

stevengj · 2019-06-06T13:12:55Z

@bkamins, String(input) looks correct to me there — there is no need to make a copy of input, because input is never modified during the execution of the function and the string is discarded on return. So avoiding the copy is good.

bkamins · 2019-06-06T13:22:54Z

The problem is that input is modified. See:

julia> using DelimitedFiles

julia> s = """a,b,c
       1,2,3"""
"a,b,c\n1,2,3"

julia> input = Vector{UInt8}(s)
11-element Array{UInt8,1}:
 0x61
 0x2c
 0x62
 0x2c
 0x63
 0x0a
 0x31
 0x2c
 0x32
 0x2c
 0x33

julia> readdlm(input, ',')
2×3 Array{Any,2}:
  "a"   "b"   "c"
 1     2     3

julia> input
0-element Array{UInt8,1}

stevengj · 2019-06-06T13:31:05Z

Oh, right, I forgot that String(bytes) nowadays truncates the bytes array.

It seems a shame to make a copy here, since the input could potentially be quite large. We could use unsafe_string(pointer(bytes), length(bytes)) as long as we do GC.@preserve. Alternatively, maybe we should have an unsafe_string(bytes) method that makes a string without truncating bytes. No, unsafe_string makes a copy as well; what we want is more like unsafe_wrap.

stevengj added the strings "Strings!" label Jun 3, 2016

vtjnash reviewed Jun 3, 2016
View reviewed changes

tkelman reviewed Jun 3, 2016
View reviewed changes

nalimilan reviewed Jun 3, 2016
View reviewed changes

StefanKarpinski mentioned this pull request Jun 3, 2016

Stringapalooza #16107

Closed

32 tasks

stevengj mentioned this pull request Jun 3, 2016

replace bytestring with String #16453

Merged

stevengj reviewed Jun 3, 2016
View reviewed changes

stevengj force-pushed the unsafe_string branch from 78fd0f9 to 1ccacbd Compare June 4, 2016 15:44

stevengj mentioned this pull request Jun 5, 2016

remove UTF-16 and UTF-32 stuff #16590

Merged

tkelman reviewed Jun 7, 2016
View reviewed changes

stevengj added 7 commits June 8, 2016 15:45

rename pointer-based string and array constructors to unsafe_string, …

77631ce

…unsafe_string_wrapper, and unsafe_array_wrapper; restore non-copying behavior of String(::Vector{UInt8}) constructor (closes JuliaLang#16470, closes JuliaLang#16713)

replace unsafe_string(pointer(buf), len) idiom with String(buf[1:n])

e821e7c

cleaner workaround for JuliaLang#16730

bd85988

rename unsafe_x_wrap to unsafe_wrap(x, ...)

2903c03

NEWS update

d5b4e2d

remove unnecessary tests for integer conversion in unsafe_wrap, and h…

5f7678e

…opefully remove occasional method ambiguity errors: overflow is already checked by convert, and sizes ≤ 0 are already checked in jl_ptr_to_array

doc fixes

6dcb602

stevengj force-pushed the unsafe_string branch from 2f49f4b to 6dcb602 Compare June 8, 2016 19:46

JeffBezanson merged commit 9ab1519 into JuliaLang:master Jun 8, 2016

stevengj deleted the unsafe_string branch June 8, 2016 23:58

kshyatt mentioned this pull request Jun 9, 2016

MethodError on latest master JuliaLang/MbedTLS.jl#45

Closed

tlnagy mentioned this pull request Oct 5, 2016

fix for deprecation warnings on 0.5 GiovineItalia/Compose.jl#223

Merged

stevengj mentioned this pull request Jun 6, 2019

readdlm(bytearray) shouldn't modify bytearray #32255

Merged

tshort mentioned this pull request Mar 1, 2022

Null termination issues in StaticString brenhinkeller/StaticTools.jl#2

Closed

restore non-copying String behavior, add unsafe_string*, ... #16731

restore non-copying String behavior, add unsafe_string*, ... #16731

Conversation

stevengj commented Jun 3, 2016 • edited Loading

stevengj commented Jun 3, 2016

tkelman commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Jun 3, 2016

Choose a reason for hiding this comment

stevengj Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

nalimilan commented Jun 3, 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

stevengj commented Jun 3, 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

stevengj Jun 3, 2016 • edited Loading

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

StefanKarpinski commented Jun 3, 2016 • edited Loading

stevengj commented Jun 4, 2016

stevengj commented Jun 5, 2016

TotalVerb commented Jun 6, 2016

stevengj commented Jun 6, 2016

TotalVerb commented Jun 6, 2016

yuyichao commented Jun 6, 2016

yuyichao commented Jun 6, 2016

TotalVerb commented Jun 6, 2016 • edited Loading

yuyichao commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jun 8, 2016

stevengj commented Jun 8, 2016

StefanKarpinski commented Jun 8, 2016

tkelman commented Jun 8, 2016

bkamins commented Jun 6, 2019

stevengj commented Jun 6, 2019

bkamins commented Jun 6, 2019

stevengj commented Jun 6, 2019 • edited Loading

stevengj commented Jun 3, 2016 •

edited

Loading

stevengj Jun 3, 2016 •

edited

Loading

stevengj Jun 3, 2016 •

edited

Loading

StefanKarpinski commented Jun 3, 2016 •

edited

Loading

TotalVerb commented Jun 6, 2016 •

edited

Loading

stevengj commented Jun 6, 2019 •

edited

Loading