safer vector<->string conversions, fixing #24388 #25241

JeffBezanson · 2017-12-22T04:25:22Z

I tried a couple things here. First, I decided that we will still want some way to wrap a String as a Vector in-place, so I made that a method of unsafe_wrap as suggested by @vtjnash. But then, there are many uses that just want to access the bytes of a string and don't do anything unsafe. To handle that I added a ~~StringBytes~~ CodeUnits AbstractVector type that exposes the bytes of a String as an immutable UInt8 vector.

My questions are (1) do we want CodeUnits and should it be exported, and (2) what should the existing functions be deprecated to? I feel the majority of uses will want CodeUnits, but that isn't an accurate deprecation since it doesn't return a Vector{UInt8}.

vtjnash · 2017-12-22T05:44:43Z

base/strings/io.jl

-IOBuffer(str::String) = IOBuffer(Vector{UInt8}(str))
-IOBuffer(s::SubString{String}) = IOBuffer(view(Vector{UInt8}(s.string), s.offset + 1 : s.offset + sizeof(s)))
+IOBuffer(str::String) = IOBuffer(unsafe_wrap(Vector{UInt8},str))
+IOBuffer(s::SubString{String}) = IOBuffer(view(unsafe_wrap(Vector{UInt8},s.string), s.offset + 1 : s.offset + sizeof(s)))


seems like it would make more sense to use a StringBytes here

vtjnash · 2017-12-22T05:46:14Z

base/strings/io.jl


-macro b_str(s); :(Vector{UInt8}($(unescape_string(s)))); end
+macro b_str(s)
+    v = unsafe_wrap(Vector{UInt8}, unescape_string(s))


This isn't performance sensitive to run the macro, but this would probably generate better runtime performance if it was making an actual copy here.

vtjnash · 2017-12-22T05:50:34Z

base/strings/string.jl

+unsafe_wrap(::Type{Vector{UInt8}}, s::String) = ccall(:jl_string_to_array, Ref{Vector{UInt8}}, (Any,), s)
+
+struct StringBytes <: AbstractVector{UInt8}
+    s::String


if this were parameterized as S <: AbstractString, this would likely be helpful for implementing readuntil (I know, somewhat random connection, but I had been strongly considering creating this type – whereas right now we typically opt for using a special-case to access bytes of String and convert everything else to a Char array)

vtjnash · 2017-12-22T05:52:11Z

base/strings/string.jl

+next(s::StringBytes, i) = (@_propagate_inbounds_meta; (s[i], i+1))
+done(s::StringBytes, i) = (@_inline_meta; i == length(s)+1)
+
+copy(s::StringBytes) = copyto!(Vector{UInt8}(uninitialized, length(s)), s)


Why are we copying immutable data? Also, seems to return the wrong type.

Yes, this should probably be a method of Vector{UInt8}. Though there's some precedent for returning a different type e.g. when copying views.

vtjnash · 2017-12-22T05:54:25Z

base/strings/string.jl

+copy(s::StringBytes) = copyto!(Vector{UInt8}(uninitialized, length(s)), s)
+
+unsafe_convert(::Type{Ptr{UInt8}}, s::StringBytes) = convert(Ptr{UInt8}, pointer(s.s))
+unsafe_convert(::Type{Ptr{Int8}}, s::StringBytes) = convert(Ptr{Int8}, pointer(s.s))


= unsafe_convert(Ptr{Int8}, s.s) and drop the definitions of pointer (why does this function still exist anyways?)

vtjnash · 2017-12-22T05:56:58Z

base/strings/util.jl

+hex2bytes(s::Union{String,AbstractVector{UInt8}}) = hex2bytes!(Vector{UInt8}(uninitialized, length(s) >> 1), s)
+
+_nbytes(s::String) = sizeof(s)
+_nbytes(s::AbstractVector{UInt8}) = length(s)


isn't this also sizeof? I thought we define that sizeof means nbytes

I was being super paranoid, but you're probably right.

vtjnash · 2017-12-22T06:01:03Z

base/c.jl

 transcode(::Type{String}, src) = String(transcode(UInt8, src))

-function transcode(::Type{UInt16}, src::Vector{UInt8})
+function transcode(::Type{UInt16}, src::AbstractVector{UInt8})


this function assumes that axes(src) === 1:length(src)

Surely it's not the only function like that... should I use Union{Vector{UInt8},StringBytes}?

I think so, let's keep this as specific as possible – we can always generalize as needed.

iamed2 · 2017-12-22T16:35:46Z

+1 for StringBytes being exported!

For the deprecation, you could deprecate to unsafe_wrap but include a message in the deprecation suggesting StringBytes?

stevengj · 2017-12-29T21:24:21Z

base/strings/io.jl

 """
-IOBuffer(str::String) = IOBuffer(Vector{UInt8}(str))
-IOBuffer(s::SubString{String}) = IOBuffer(view(Vector{UInt8}(s.string), s.offset + 1 : s.offset + sizeof(s)))
+IOBuffer(str::String) = IOBuffer(unsafe_wrap(Vector{UInt8}, str))


Probably also IOBuffer(s::StringBytes) = IOBuffer(s.s)

I have a different question but related to the same line. Why unsafe_wrap is used here - for efficiency? I am asking, because it seems that this should work with StringBytes equally well.

I tried using StringBytes and a few things broke that I didn't feel like dealing with. The type IOBuffer actually implies that a Vector{UInt8} will be used:

julia> IOBuffer Base.GenericIOBuffer{Array{UInt8,1}}

so returning a GenericIOBuffer{StringBytes} instead can cause surprises.

stevengj · 2017-12-29T21:26:58Z

base/strings/string.jl

+Wrap a `String` (without copying) in an immutable vector-like object that accesses the bytes
+of the string's representation.
+"""
+struct StringBytes <: AbstractVector{UInt8}


<: DenseVector{UInt8}, and include strides methods?

JeffBezanson · 2017-12-31T16:02:35Z

@StefanKarpinski @bkamins Approve?

bkamins · 2017-12-31T17:07:01Z

Thanks! I understand the target functionality is:

convert(::Vector{UInt8}, ::String) is removed;
Vector{UInt8}(::String) copies data;
unsafe_wrap gives Vector{UInt8} giving access to String data;
StringBytes type gives a read-only version, the drawback is that it is only DenseVector{UInt8}.

I think it is very good and StringBytes should be exported. Deprecation approach was clear to me.

StefanKarpinski · 2017-12-31T18:22:15Z

We might want to hold off on exporting StringBytes. It feels a bit ad hoc and like it could in the future be a special case of something more general. E.g. we were discussing making codeunits return just such an object and writing length(codeunits(s)) instead of ncodeunits(s) and eltype(codeunits(s)) instead of what is currently codeunit(s), etc. We'd want to be able to return an immutable vector-like type for that with different kinds of code units in it.

StefanKarpinski · 2017-12-31T18:23:07Z

If you want me to, I could make a PR where codeunits(s) returns a StringBytes object instead.

stevengj · 2017-12-31T19:11:00Z

length(codeunits(s)) seems unnecessarily expensive if it requires one to create an intermediate StringBytes object. Or is that cheap nowadays?

StefanKarpinski · 2017-12-31T19:21:28Z

Or is that cheap nowadays?

Yes, that's my concern as well. I'm not sure if it's cheap yet. cc @Keno, @vtjnash

Keno · 2017-12-31T19:25:31Z

We're pretty good at eliding allocations these days, so I suspect it'd be fine, but I'd recommend verifying in the cases you're interested in. One thing in particular is that we can only elide allocations if both the allocation and all its uses are inlined into one function.

vtjnash · 2017-12-31T20:11:54Z

If you want me to, I could make a PR where codeunits(s) returns a StringBytes object instead.

If so, it seems like the name is wrong. Either it would be specifically a stringbytes(s) function (returning a byte iterator) or StringCodeUnits (representing iteration over code units).

StefanKarpinski · 2017-12-31T22:58:46Z

Yes, clearly the name StringBytes would not be appropriate for a more generic type providing access to the code units of a string.

JeffBezanson · 2018-01-02T16:32:46Z

I think we don't want string types to have to define codeunits, essentially because returning a container is more difficult than returning an integer. For example it's easy to have an inefficient implementation (e.g. that constructs a new Vector on every call), or we might see multiple redundant types all similar to StringBytes.

I can rename this CodeUnits and return it as a default implementation of codeunits. How's that?

StefanKarpinski · 2018-01-02T16:36:32Z

I think if the type could be called CodeUnits{UInt8} that would be good. Otherwise, we can just continue to call this StringBytes for now and in the future if CodeUnits{UInt8} or ImmutableVector{UInt8} becomes a more general version of this and StringBytes an alias.

add `CodeUnits` and `codeunits` fixes #24388

part of #24388

JeffBezanson · 2018-01-02T19:07:40Z

OK, see how this grabs you.

stevengj · 2018-01-02T19:46:45Z

test/strings/basic.jl

+let v = unsafe_wrap(Vector{UInt8}, "abc")
+    s = String(v)
+    @test_throws BoundsError v[1]
+    push!(v, UInt8('x'))


I'm confused. Why doesn't this line throw an error?

It doesn't need to. When push! is called new storage is allocated for the vector (also as a String in case you want to make more strings from it).

stevengj · 2018-01-02T19:48:05Z

base/strings/string.jl

 String(s::Symbol) = unsafe_string(unsafe_convert(Ptr{UInt8}, s))

-(::Type{Vector{UInt8}})(s::String) = ccall(:jl_string_to_array, Ref{Vector{UInt8}}, (Any,), s)
+unsafe_wrap(::Type{Vector{UInt8}}, s::String) = ccall(:jl_string_to_array, Ref{Vector{UInt8}}, (Any,), s)


Should we have unsafe_wrap(::Type{Array}, s) = unsafe_wrap(Vector{UInt8}, s)?

StefanKarpinski · 2018-01-02T20:30:41Z

This looks good to me. I guess the wrapping of string object itself is fairly essential here, which inherently couples the CodeUnits type to the strings that it wraps. I guess it's possible that we might at some point be able to return a generic immutable code unit vector object, but that seems fairly distant, so probably 2.0 material in any case.

JeffBezanson · 2018-01-02T20:51:35Z

be able to return a generic immutable code unit vector object

What's the difference? This should work for anything that implements ncodeunits and codeunit.

StefanKarpinski · 2018-01-02T21:00:11Z

What's the difference?

Just that CodeUnits{UInt8,String} is string-specific whereas we could potentially just want an ImmutableVector{UIn8} type and return that. The ideal structuring here would be inverted: a String would be a wrapper around an ImmutableVector{UInt8} object, which is what we would return from codeunits(::String). Given that we're not going to have that for 1.0, I think this arrangement is good.

JeffBezanson added the strings "Strings!" label Dec 22, 2017

vtjnash reviewed Dec 22, 2017

View reviewed changes

JeffBezanson mentioned this pull request Dec 23, 2017

implement sizeof for SubArray #25258

Merged

JeffBezanson force-pushed the jb/vectorstring branch 2 times, most recently from ebb7d86 to 494705e Compare December 23, 2017 06:41

remove some uses of Vector{UInt8}(::String)

41697f9

JeffBezanson force-pushed the jb/vectorstring branch from 494705e to 76b1330 Compare December 29, 2017 20:59

stevengj reviewed Dec 29, 2017

View reviewed changes

JeffBezanson force-pushed the jb/vectorstring branch from 76b1330 to 1cae074 Compare December 30, 2017 16:08

JeffBezanson added 2 commits January 2, 2018 13:21

deprecate Vector<->String conversion in favor of something safer

7a44c04

add `CodeUnits` and `codeunits` fixes #24388

prevent modifying vector after in-place conversion to string

7bce3b1

part of #24388

JeffBezanson force-pushed the jb/vectorstring branch from 1cae074 to 7bce3b1 Compare January 2, 2018 19:05

stevengj reviewed Jan 2, 2018

View reviewed changes

JeffBezanson changed the title ~~RFC: safer vector<->string conversions, fixing #24388~~ safer vector<->string conversions, fixing #24388 Jan 3, 2018

JeffBezanson merged commit 2043060 into master Jan 3, 2018

JeffBezanson deleted the jb/vectorstring branch January 3, 2018 18:48

maleadt added a commit to JuliaGPU/CUDAdrv.jl that referenced this pull request Jan 4, 2018

Fix after JuliaLang/julia#25241

eea3b8e

stevengj mentioned this pull request Jan 24, 2018

codeunits and ncodeunits JuliaLang/Compat.jl#474

Merged

nalimilan mentioned this pull request Oct 14, 2018

Conversion of String to Vector{UInt8} #24388

Closed

KristofferC mentioned this pull request May 13, 2024

New copystring function #54424

Closed

jakobnissen mentioned this pull request May 14, 2024

Add takestring!(x) to create a string from the content of x, emptying it #54372

Merged

jakobnissen mentioned this pull request May 27, 2024

Switch supertype of CodeUnits from DenseVector to AbstractVector #54002

Open

Uh oh!

safer vector<->string conversions, fixing #24388 #25241

safer vector<->string conversions, fixing #24388 #25241

Uh oh!

Conversation

JeffBezanson commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamed2 commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevengj Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Dec 31, 2017

Uh oh!

bkamins commented Dec 31, 2017

Uh oh!

StefanKarpinski commented Dec 31, 2017

Uh oh!

StefanKarpinski commented Dec 31, 2017

Uh oh!

stevengj commented Dec 31, 2017

Uh oh!

StefanKarpinski commented Dec 31, 2017

Uh oh!

Keno commented Dec 31, 2017

Uh oh!

vtjnash commented Dec 31, 2017

Uh oh!

StefanKarpinski commented Dec 31, 2017

Uh oh!

JeffBezanson commented Jan 2, 2018

Uh oh!

StefanKarpinski commented Jan 2, 2018

Uh oh!

JeffBezanson commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanKarpinski commented Jan 2, 2018

Uh oh!

JeffBezanson commented Jan 2, 2018

Uh oh!

StefanKarpinski commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

JeffBezanson commented Dec 22, 2017 •

edited

Loading

stevengj Dec 29, 2017 •

edited

Loading

StefanKarpinski commented Jan 2, 2018 •

edited

Loading