Skip to content
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.

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

make IOBuffer(array) writable by default #24430

Closed
wants to merge 2 commits into from

Conversation

bicycle1885
Copy link
Member

IOBuffer(UInt8[]) is not writable by default while IOBuffer() is. This looks inconsistent to me, and so I'd like to make IOBuffer(array) writable by default. IOBuffer(string) is kept as-is because making it writable may result in an expected behavior.

@ararslan ararslan added domain:io Involving the I/O subsystem: libuv, read, write, etc. needs news A NEWS entry is required for this change labels Nov 1, 2017
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This looks inconsistent to me

Why is this inconsistent? open is also usually read-only by default.

@bicycle1885
Copy link
Member Author

One may expect IOBuffer() is a shorthand of IOBuffer(UInt8[]) but actually it isn't due to the difference of the writable flag. open is a function to open a file and so it is not directly related to IOBuffer's constructor. Also, in most cases, IOBuffer(array) calls are with writable=true:

~/v/julia (master|…) $ rg '=.*IOBuffer\(' base | rg -v "IOBuffer\(\)" | rg -v "\w+\(.*="
base/io.jl:julia> buf = IOBuffer("    text")
base/iobuffer.jl:    b = IOBuffer(StringVector(32), readable, writable)
base/printf.jl:    fmt = IOBuffer(fmt_len)
base/dates/io.jl:    io = IOBuffer(Vector{UInt8}(bufsize), true, true)
base/strings/basic.jl:    out = IOBuffer(StringVector(endof(s)),true,true)
base/strings/basic.jl:    out = IOBuffer(StringVector(endof(s)),true,true)
base/strings/io.jl:    s = IOBuffer(StringVector(size), true, true)
base/strings/io.jl:    s = IOBuffer(StringVector(tostr_sizehint(xs[1])), true, true)
base/strings/io.jl:julia> io = IOBuffer("Haho");
base/strings/io.jl:    buf = IOBuffer(StringVector(endpos), true, true)
base/strings/util.jl:    out = IOBuffer(StringVector(floor(Int, 1.2sizeof(str))), true, true)

@samoconnor
Copy link
Contributor

related #24526

@StefanKarpinski
Copy link
Sponsor Member

Bump – can we make some progress here? @JeffBezanson has already approved so my merge finger is getting itchy...

@stevengj
Copy link
Member

stevengj commented Feb 1, 2018

Read-only seems like a safer default here. (Note that most of the writable usages in Base employ the undocumented StringVector.) @vtjnash's analogy with open makes sense to me. Note that making it writable changes the behavior of take!, for example, so this is a breaking change.

Most of the cases where you pass a writable array seem to be just cases where you want a size hint, and it would be better in this case to actually have a sizehint parameter (e.g. so that it uses a StringVector).

Now that keywords are fast, can we just change the IOBuffer arguments into keywords? IOBuffer(array, writable=true) is a lot more readable than IOBuffer(array, true, true), and then we could add IOBuffer(sizehint=10).

@stevengj stevengj added the kind:breaking This change will break code label Feb 1, 2018
@StefanKarpinski StefanKarpinski added the status:triage This should be discussed on a triage call label Feb 1, 2018
@StefanKarpinski StefanKarpinski changed the title make IOBuffer(array) writable by defualt make IOBuffer(array) writable by default Feb 1, 2018
@StefanKarpinski
Copy link
Sponsor Member

From triage: this should behave like open and use the same open_flags keyword pre-processing as open does with a few additional options like sizehint. There are a few other optional positional arguments that we may not want to expose as part of the public API of IOBuffer.

@bicycle1885 bicycle1885 closed this Feb 7, 2018
@JeffBezanson JeffBezanson removed the status:triage This should be discussed on a triage call label Feb 8, 2018
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. kind:breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants