avoid a copy in readuntil #19946
avoid a copy in readuntil #19946
Conversation
| a `Vector{UInt8}`. | ||
| """ | ||
| readuntil_string(s::IO, delim::UInt8) = String(readuntil(s, delim)) | ||
| function readuntil_string(s::IOStream, delim::UInt8) |
There was a problem hiding this comment.
Would it be better to make this readuntil(s::IOStream, delim::UInt8) instead of giving this a new name? Also other documented methods using readuntil return a string.
I would rather rename the old readuntil to readbytes_until or read_until since it's not part of documented API anyway.
There was a problem hiding this comment.
I agree, let's not add _string functions to the API at a time when we're rather trying to clean it (cf. removal of takebuf_string).
There was a problem hiding this comment.
Okay, but that would be a breaking change.
(Though right now I can only find one package using the UInt8 version of readuntil.)
There was a problem hiding this comment.
if readuntil(io, delim::UInt8) is changed to return a string, people could just use Vector{UInt8}(readuntil(io, delim)) to get a UInt8 array (and it will still work in Julia 0.5), so maybe we won't need to define a separate method.
There was a problem hiding this comment.
Hmm. And couldn't you just do io <: IOStream && ccall(:jl_readuntil, ... inside the existing readuntil?
There was a problem hiding this comment.
@nalimilan, sure, but the Julian way to do this is to write methods rather than explicit type checks. In this case, I wrote a non-exported method.
There was a problem hiding this comment.
Ah, sorry, due to the detailed docstring I thought it was exported. If it's not then I guess any approach is OK.
There was a problem hiding this comment.
I also thought it was exported, better to leave it as it is than to make breaking change.
There was a problem hiding this comment.
I updated to make it clearer that this is not exported, replacing the docstring with a comment
|
@nanosoldier |
| """ | ||
| readuntil_string(s::IO, delim::UInt8) = String(readuntil(s, delim)) | ||
| function readuntil_string(s::IOStream, delim::UInt8) | ||
| ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, delim, 1) |
There was a problem hiding this comment.
This is changed in #19944, the new version is
ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8, UInt8), s.ios, delim, 1, 0)where the last 0 is for chomp=false.
There was a problem hiding this comment.
depends on which PR gets merged first 😉
| Like `readuntil(s, delim)`, but returns a `String` rather than | ||
| a `Vector{UInt8}`. | ||
| """ | ||
| readuntil_string(s::IO, delim::UInt8) = String(readuntil(s, delim)) |
There was a problem hiding this comment.
The generic version can go in io.jl.
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels |
…ction is not exported
|
Should be ready to merge? |
This avoids a copy of the string data in
readuntil(s::IO, delim::Char)whendelimis ASCII andsis a file.