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

Rename AbstractIOBuffer, it's not an abstract type #17360

Closed
wants to merge 3 commits into from

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Jul 10, 2016

rename AbstractIOBuffer to IOBufferBase
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.

@vtjnash
Copy link
Member

vtjnash commented Jul 10, 2016

-1 The code isn't parameterized to handle this correctly. This seems to be code churn with no benefit.

@tkelman
Copy link
Contributor Author

tkelman commented Jul 10, 2016

The benefit is the current AbstractIOBuffer isn't an abstract type at all. That's just silly, and actively confusing, naming. We could @deprecate_binding AbstractIOBuffer GeneralIOBuffer if you'd prefer that.

(osx travis failure was a seemingly unrelated freeze, log backed up to https://gist.github.com/080189cb8f707e08c404d4a57f927fad and restarted)

@TotalVerb
Copy link
Contributor

Can't this be called IOBuffer? The constructor can be made to default to Vector{UInt8}. The only thing that should break are functions specialized on IOBuffer that suddenly accept all kinds of IOBuffer{T}s, which isn't necessarily a bad thing.

@tkelman
Copy link
Contributor Author

tkelman commented Jul 10, 2016

I suspect it could but I wasn't sure exactly what that would look like.

@tkelman
Copy link
Contributor Author

tkelman commented Jul 11, 2016

Suggestions? This seemed the least disruptive immediate resolution of the misleading naming. @stevengj you had some thoughts in february?

@vtjnash
Copy link
Member

vtjnash commented Jul 11, 2016

If you just don't want the name, the most correct change IMO would be just s/AbstractIOBuffer/IOBufferBase/g

Definitely don't add an actual abstract type. That's completely wrong (the abstract type interface this implements is IO. The type itself is abstracting over the storage backing, not the interface.).

FWIW, it's probably wrong to call AbstractIOBuffer a concrete type. In the part of our manual that introduce parametric types, it is called an abstract type: What does Point by itself mean? It is an abstract type... (http://docs.julialang.org/en/release-0.4/manual/types/#parametric-composite-types)

@tkelman
Copy link
Contributor Author

tkelman commented Jul 11, 2016

We should probably be more distinct in our wording then, since abstract is a keyword we should avoid using that term elsewhere. I'd say something like "parameterized type family". I'll modify this to be a @deprecate_binding rename to IOBufferBase then.

@vtjnash
Copy link
Member

vtjnash commented Jul 11, 2016

Even more confusingly, from talking with carnaval, abstract IO is probably actually describing a concrete type from the type-system standpoint, since it has no unbound type-parameters. Thus, if we allowed construction of non-leaf types, it might be a candidate for being an instantiatable type (cross-ref #17086 discussion as well)

@stevengj
Copy link
Member

stevengj commented Jul 11, 2016

Because the abstract parametric type shares the same name with the concrete parametric type for a given T, the usual Julia convention is to not use the name AbstractFoo in that case, and it is weird to make an exception here. (We don't say AbstractComplex even though, by that definition, Complex is an abstract type.)

I tend to favor just calling this IOBuffer, as @TotalVerb suggests, and seeing where the cards fall.

@tkelman
Copy link
Contributor Author

tkelman commented Jul 11, 2016

Complex.abstract is false though so I don't think that's the best terminology to use for unbound parameters.

this is a bit confusing and we don't have a great word for it,
but since Point wasn't declared with abstract here we probably
shouldn't use that term
ref discussion at #11554 (comment)
it might be better to rename this to IOBuffer but I'm not sure how to make that work
@tkelman
Copy link
Contributor Author

tkelman commented Jul 12, 2016

I also like the sound of just calling the parameterized version IOBuffer but I'm not positive what the patch to do that correctly would look like.

@tkelman tkelman changed the title Make AbstractIOBuffer actually an abstract type Rename AbstractIOBuffer, it's not an abstract type Jul 12, 2016
@kshyatt kshyatt added the types and dispatch Types, subtyping and method dispatch label Aug 1, 2016
@ggggggggg
Copy link
Contributor

I just looked at iobuffer.jl to figure out how to implement IO and was very confused by the name AbstractIOBuffer for a few minutes. I support this change, or even changing it to IOBuffer, though I understand from the thread that that is harder. I think there is benefit in changing the name to make the code easier to read.

@StefanKarpinski
Copy link
Member

The fact that AbstractIOBuffer is not an abstract type is totally ridiculous and must be fixed by 1.0.

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Apr 18, 2017
@StefanKarpinski
Copy link
Member

Resolved: renaming it to GenericIOBuffer.

@tkelman tkelman closed this Jul 13, 2017
@tkelman tkelman deleted the tk/abstractiobuffer branch July 13, 2017 19:15
JeffBezanson added a commit that referenced this pull request Jul 14, 2017
rename AbstractIOBuffer => GenericIOBuffer (close #17360)
jeffwong pushed a commit to jeffwong/julia that referenced this pull request Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants