add sizehint keyword argument to IOBuffer #25944
Conversation
base/iobuffer.jl
Outdated
| size = maxsize == typemax(Int) ? 32 : Int(maxsize) | ||
| maxsize::Integer=typemax(Int), | ||
| sizehint::Union{Integer,Nothing}=nothing) | ||
| size = maxsize != typemax(Int) ? Int(maxsize) : sizehint !== nothing ? Int(sizehint) : 32 |
There was a problem hiding this comment.
It seems like sizehint should take precedence over maxsize here.
There was a problem hiding this comment.
e.g. consider a case where maxsize is 2GB (because the buffer will be sent someplace 32-bit) but sizehint is 8. We probably don't want to allocate 2GB.
There was a problem hiding this comment.
I forgot to fix that. Now fixed. Thank you.
base/strings/basic.jl
Outdated
| @@ -525,7 +525,7 @@ function map(f, s::AbstractString) | |||
| end | |||
|
|
|||
| function filter(f, s::AbstractString) | |||
| out = IOBuffer(StringVector(sizeof(s)), read=true, write=true) | |||
| out = IOBuffer(sizehint=sizeof(s), read=true, write=true) | |||
There was a problem hiding this comment.
We no longer have to pass read=true, write=true, since this is the default when no data buffer is passed to the constructor. (Similarly elsewhere.)
There was a problem hiding this comment.
i.e. just call IOBuffer(sizehint=sizeof(s)).
base/strings/io.jl
Outdated
| @@ -100,7 +100,7 @@ tostr_sizehint(x::Float32) = 12 | |||
|
|
|||
| function print_to_string(xs...; env=nothing) | |||
| # specialized for performance reasons | |||
| s = IOBuffer(StringVector(tostr_sizehint(xs[1])), read=true, write=true) | |||
| s = IOBuffer(sizehint=tostr_sizehint(xs[1]), read=true, write=true) | |||
There was a problem hiding this comment.
Shouldn't there be an isempty(xs) && return "" check? Or is this an internal function that can never be called on an empty argument list, in which case there should be an @assert !isempty(xs)?
base/strings/io.jl
Outdated
| @@ -81,7 +81,7 @@ julia> sprint(showcompact, 66.66666) | |||
| ``` | |||
| """ | |||
| function sprint(f::Function, args...; context=nothing, sizehint::Integer=0) | |||
| s = IOBuffer(sizehint=sizehint, read=true, write=true) | |||
| s = IOBuffer(sizehint=sizehint) | |||
| # specialized version of truncate(s,0) | |||
| s.size = 0 | |||
| s.ptr = 1 | |||
There was a problem hiding this comment.
Can't the these three lines be deleted?
base/strings/io.jl
Outdated
| @@ -100,7 +100,7 @@ tostr_sizehint(x::Float32) = 12 | |||
|
|
|||
| function print_to_string(xs...; env=nothing) | |||
| # specialized for performance reasons | |||
| s = IOBuffer(sizehint=tostr_sizehint(xs[1]), read=true, write=true) | |||
| s = IOBuffer(sizehint=tostr_sizehint(xs[1])) | |||
| # specialized version of truncate(s,0) | |||
| s.size = 0 | |||
| s.ptr = 1 | |||
There was a problem hiding this comment.
Can't the these three lines be deleted?
| @@ -96,6 +96,9 @@ tostr_sizehint(x::Float64) = 20 | |||
| tostr_sizehint(x::Float32) = 12 | |||
|
|
|||
| function print_to_string(xs...; env=nothing) | |||
| if isempty(xs) | |||
| return "" | |||
| end | |||
There was a problem hiding this comment.
isempty(xs) && return "" would be more idiomatic (and shorter).
There was a problem hiding this comment.
I think it is a personal taste rather than an idiom. I'd like to use if in this kind of case because it is easier to read control flow.
There was a problem hiding this comment.
The && syntax is widely used in Base to check for small corner cases at the beginning of functions. Hence I would say it is idiomatic in Base. (The word “idiomatic” refers to choices that are questions of taste/usage rather than grammar.)
Obviously the two are technically equivalent, so I won’t insist.
There was a problem hiding this comment.
(The question in cases like this is not your personal taste, but rather the dominant style of the project being patched.)
There was a problem hiding this comment.
But I can find lots of if <condition> <newline> <single statement> <newline> end usage in Base (I don't know stats on which is more dominant) and cannot find a guideline on the style, so I used the words "personal taste" here. Anyway, I think this is off-topic.
There was a problem hiding this comment.
Both styles are definitely acceptable.
This adds
sizehintkeyword argument toIOBufferas suggested by @stevengj in #25872 (comment).