Simplify takebuf() API

[nalimilan]

This implements one of the Stringapalooza tasks scheduled for 0.6 (#16107 and #16058 (comment)): takebuf_array is now simply takebuf, and takebuf_string(b) is now String(takebuf(b)).

Points to discuss:

• I've removed the jl_takebuf_string C function, which was used for IOStream method and performed the conversion from array to string in C. I don't think it should be a performance issue, but could someone confirm that?
• I've kept the name takebuf, but maybe another one is appropriate now? Could it be a method of a more generic function like collect or Array?
• There's already a String(b::AbstractIOBuffer) method with a similar behavior, but it only works on seekable streams. One difference is that it doesn't call unmark, maybe there are others. Probably doesn't affect this PR, but I thought I would mention it.

Fixes #8575.

[vtjnash]

takebuffer? or buffer!?

[dhoegh]

What about take!? It does not seem in conflict with the current uses. The help list:

help?> take!
search: take! take takebuf_array takebuf_string StackOverflowError istaskdone stacktrace StackTraces

  take!(c::Channel)

  Removes and returns a value from a Channel. Blocks till data is available.

  take!(rr::RemoteChannel, args...)

  Fetch value(s) from a remote channel, removing the value(s) in the processs.

[ararslan]

I like the idea of overloading take! for this purpose but IIRC that function was recently moved to a new Base.Iterators module, so you'd have to call it like Iterators.take!. I suppose it doesn't really matter, but to me the name Iterators seems a little odd in the context of working with buffers.

[nalimilan]

take! sounds nice, but as @vtjnash told me today I/O functions don't end with a ! in general even when they mutate the input stream. So it would have to be take, which has a very different meaning.

@vtjnash Why do you suggest buffer! with an ending !?

[dpsanders]

Can't it just be string(buf) where buf::IOBuffer?

[yuyichao]

string(buf)

No, string is roughly how you show (edit: print) the object.

[dpsanders]

There is repr for that.

[yuyichao]

No that's yet another more verbose representation of the object. http://docs.julialang.org/en/release-0.5/stdlib/strings/?highlight=repr

[nalimilan]

It could be String(b), but as I said it already exists and has a slightly different behavior. I think the rationale it that a constructor doesn't look like it should mutate the input argument (by calling unmark!).

I've just re-discovered that there was an issue for this naming debate, but not with many proposals: #8575.

[dhoegh]

@ararslan as far as I can see only take and not take! is deprecated in https://github.com/JuliaLang/julia/pull/18839/files#diff-7904f4ddd9158030529e0ed5ee8707eeR1031.

[ararslan]

@dhoegh Oh you're right—I mistakenly thought take! was the mutating form of Iterators.take, but that seems to not be the case. In that case, +1 to take!.

[stevengj]

+1 for take!. This isn't an I/O function in the same sense as read or write, so I think ! seems appropriate.

[nalimilan]

So is the general preference for take!?

Something that escapes me is why the docs say "Obtain the contents of an IOBuffer as an array, without copying". Looking at the code, it seems to me that a copy is always made, even when it could be avoided by reusing the data field. I would be more inclined to use take! if that field was indeed reused.

[vtjnash]

If it is seekable, writable, and backed by an Array{UInt8}, then it is no-copy. I agree that the seekable condition seems somewhat extraneous.

[nalimilan]

Thanks, I hadn't noticed that the newly allocated array was of length 0 in the best case.

I've updated the PR to use take!. I've also renamed jl_takebuf_array and ios_takebuf to jl_take_buffer and ios_take_buffer, which seems more consistent.

[nalimilan]

Should we merge now?

[nalimilan]

I'll merge tomorrow barring further suggestions or objections.

[nalimilan]

Phew! It was hard to get into a mergeable state, deprecated.jl is such a busy place these days...

[s2maki]

This deprecation warning is sometimes wrong (and results in an error in those cases). When takebuf_string(v) is passed a UInt8 vector, it reports that String(take!(v)) is the replacement, but take!(v) doesn't accept a UInt8 vector. It should just be String(v) in that case.

[yuyichao]

In which version did passing a Vector{UInt8} to takebuf_string actually work?

[nalimilan]

At least the Vector{UInt8} method (if it ever existed) wasn't removed in this PR.