add sizehint keyword argument to IOBuffer#25944
Conversation
| 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.
|
|
||
| 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)).
| 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)?
| 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?
| 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?
| 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.
3 participants
This adds
sizehintkeyword argument toIOBufferas suggested by @stevengj in #25872 (comment).