RFC: allow IOBuffer to be parameterized and remove start/stop_reading #11554
Conversation
| @@ -1,23 +0,0 @@ | |||
| // This file is a part of Julia. License is MIT: http://julialang.org/license | |||
There was a problem hiding this comment.
this deletion looks unrelated, missed from a0e503e ?
There was a problem hiding this comment.
yeah, looks like my branches got mixed up in the rebase
There was a problem hiding this comment.
I think strictly speaking no, just this file deletion could have been committed to master by itself at any time.
vtjnash
changed the title
|
👍 💯 to the start_reading change. The IOBuffer change seems like an awful lot of effort just to support read-only IOBuffers of substrings of ASCII or UTF-8. I haven't seen sufficient motivation for that. |
| end | ||
| end | ||
|
|
||
| wait_close(x) = if isopen(x) stream_wait(x,x.closenotify); end | ||
| wait_close(x) = wait_readnb(x, typemax(Int)) |
There was a problem hiding this comment.
self note: revert this line
vtjnash
force-pushed
the
jn/iobuffer_parameterized
branch
from
63cda45
to
9de4a9a
Compare
I really need to go through and split up these commits into bug fixes and changes. The actual parameterization of the IOBuffer is done in very few changes (most just changing of typed fields from IOBuffer to SimpleIOBuffer). The rest is either unrelated (but got mixed up in my local branches), or bugfixes / code simplifications. I think the more useful motivating case for this is being able to make IOBuffers backed by SubArrays or SharedArrays. |
|
I wouldn't underestimate the annoyance of having both |
|
I don't know, I think this might be quite useful for certain types of operations, say where you've got data streaming into buffers, a buffer is filled, and then you start filling another buffer... normally trying to process the data in those buffers is a pain, but this would hide that, I think... and could be a big win for the sorts of stuff that I do. I don't care what the name is, just that the functionality is there, and that use can use these anywhere you'd read from an IOBuffer currently (without causing type instability or performance issues for code that doesn't use the new type IOBuffers...) |
vtjnash
force-pushed
the
jn/iobuffer_parameterized
branch
2 times, most recently
from
4ff1b04
to
507d17e
Compare
|
@JeffBezanson i've cleaned up the commits and done the renaming you suggested. i agree that is better. I initially though the abstract type would be useful for clients, but now realize that was largely false, since the primarily useful abstract type for annotation would be |
vtjnash
force-pushed
the
jn/iobuffer_parameterized
branch
from
507d17e
to
38888d5
Compare
|
travis is now ok with this (modulo the OOM death on one of the attempts). any final thoughts before merging? |
|
FWIW, LGTM 👍 |
|
Argh: spoke too soon, but this still can be merged and I can submit a PR later for the minor thing I just noticed. |
allow IOBuffer to be parameterized, and remove the need for explicit start/stop_reading in user code
and rename the parameterized version to `GeneralIOBuffer` ref discussion at #11554 (comment) Luckily very little code in base and no code in registered packages is using the constructor for this type, only in dispatch (or in `precompile` calls) so this change should in theory not be very noticeable. After we branch for release-0.5 we should consider removing the parameterization and just call the type `IOBuffer`, since it doesn't look like anyone ever uses any other type for the `data` field.
ref discussion at #11554 (comment) it might be better to rename this to IOBuffer but I'm not sure how to make that work
ref #9888
ref #1925
(note: these two pull requests have gotten mixed up together in my local repository commits. i'll definitely fix that and sort them into proper commits before merging this)