Skip to content

avoid a copy in readuntil #19946

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

Merged
merged 3 commits into from
Jan 21, 2017
Merged

avoid a copy in readuntil #19946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2c4426
avoid a copy in readuntil
stevengj Jan 9, 2017
fce5d55
fix typo
stevengj Jan 10, 2017
e324f26
move generic readuntil_string to io.jl, make it clearer that this fun…
stevengj Jan 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion base/io.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,13 @@ function read(s::IO, ::Type{Char})
return Char(c)
end

# readuntil_string is useful below since it has
# an optimized method for s::IOStream
readuntil_string(s::IO, delim::UInt8) = String(readuntil(s, delim))

function readuntil(s::IO, delim::Char)
if delim < Char(0x80)
return String(readuntil(s, delim % UInt8))
return readuntil_string(s, delim % UInt8)
end
out = IOBuffer()
while !eof(s)
Expand Down
5 changes: 5 additions & 0 deletions base/iostream.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ function readuntil(s::IOStream, delim::UInt8)
ccall(:jl_readuntil, Array{UInt8,1}, (Ptr{Void}, UInt8, UInt8), s.ios, delim, 0)
end

# like readuntil, above, but returns a String without requiring a copy
function readuntil_string(s::IOStream, delim::UInt8)
Copy link
Contributor

@mpastell mpastell Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, but that would be a breaking change.

(Though right now I can only find one package using the UInt8 version of readuntil.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. And couldn't you just do io <: IOStream && ccall(:jl_readuntil, ... inside the existing readuntil?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, due to the detailed docstring I thought it was exported. If it's not then I guess any approach is OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also thought it was exported, better to leave it as it is than to make breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated to make it clearer that this is not exported, replacing the docstring with a comment

ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, delim, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

depends on which PR gets merged first 😉

end

function readline(s::IOStream)
ccall(:jl_readuntil, Ref{String}, (Ptr{Void}, UInt8, UInt8), s.ios, '\n', 1)
end
Expand Down