Specialize BufList::copy_to_bytes

[vorner]

Some implementations of the Buf trait have an optimized version (for
example Bytes) of copy_to_bytes, opportunistically use that one.

For some background, I was implementing a similar type and had a look there and noticed it is not implemented. Or is there some reason why it shouldn't be implemented?

[seanmonstar]

Good idea! The implementation looks right to me (though trickier than I assumed before looking). What do you think of adding a few tests since it's tricky?

• If only containing a single Bytes, the pointer should be the same before and after.
  • That it popped the buf if the length is the same.
• That it handles a single buf that's longer.
• That it handles multiple bufs.

[vorner]

OK, that makes sense. And yes, these things usually come with more corner cases than expected (this whole implementation doesn't even assume empty buffers are possible somewhere in the middle).

During the tests I've noticed this implementation doesn't panic if the request is longer than all the buffers put together. That is probably allowed, because the advance allows for non-panicking and copy_to_bytes doesn't have any note about panicking. But I think it would be better if it panicked. There are few possibilities:

• Leave it as it is (+ add test for that it doesn't panic and takes the whole rest)
• Add a trivial test at the top of the relevant block. But that one will call .remaining() which iterates through all the buffers one more time, which could be a bit slower.
• Do the check after the copying is done, as the Bytes::remaining() is likely something much faster. The downside is, the data has already been consumed so we could either put it back (push_front), at the cost of added code complexity, or say that after a panic, the structure is in some unknown state anyway.
• Port keeping pre-computed remaining length from the version I've written. The downside is added code complexity.
• Use that ⬆️ as a dependency. But it's a 0.1 crate (though I don't expect breaking the API of this particular type), it's additional dependency, and your code has some actual benchmarks around this type.

So I'd like to know which option you'd prefer.

[seanmonstar]

The default implementation does panic: https://docs.rs/bytes/1.0.1/src/bytes/buf/buf_impl.rs.html#816

It seems like we can just add the check to the _ => branch, since the other 2 would be delegating to another implementation that can do that same check.

[vorner]

The default implementation does panic

I know. I was just saying that an implementation is probably not required to panic in that situation.

As for the check, I was probably stressing too much over the fact that remaining iterates over all the inner buffers while IMO remaining should be cheap… but then, the default implementation did it too.

[seanmonstar]

Excellent work, thank you!