Uh oh!
There was an error while loading. Please reload this page.
Add {IoSlice, IoSliceMut}::advance - #62987
Conversation
rust-highfive
commented
Jul 25, 2019
(rust_highfive has picked a reviewer for you, use r? to override) |
Uh oh!
There was an error while loading. Please reload this page.
alexcrichton
commented
Jul 26, 2019
Thanks for the PR! As a convenience function though I would imagine that wouldn't this be more useful if defined on something like |
Thomasdezeeuw
commented
Jul 26, 2019
I agree, that is what I need for rust-lang/futures-rs#1741 as well. I'm fine with changing it. But where would I put the code for such a change? In the |
alexcrichton
commented
Jul 26, 2019
Perhaps something like: implIoVec<'_>{pubfnadvance(slice:&mut[IoVec]){/* ... */}}implIoVecMut<'_>{pubfnadvance(slice:&mut[IoVecMut]){/* ... */}}or something like that? |
Thomasdezeeuw
commented
Jul 28, 2019
Looks good, I'll try to update the pr tomorrow. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
This might need expanding.
There was a problem hiding this comment.
Yeah I'd recommend going into detail here and explaining how the vector elements of the slice may be modified to adjust the size as noted by advance
There was a problem hiding this comment.
What do you think of what I've added?
Thomasdezeeuw
commented
Jul 31, 2019
I've addressed all comments and updated the API to return the truncated slice. |
rust-highfive
commented
Jul 31, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
The documentation here I think may also want to mention that this API panics if you advance beyond the cumulative size of all buffers.
There was a problem hiding this comment.
But it doesn't panic if you go beyond the cumulative size, see the io_slice_advance_beyond_total_length test.
There was a problem hiding this comment.
Yeah I'd recommend going into detail here and explaining how the vector elements of the slice may be modified to adjust the size as noted by advance
There was a problem hiding this comment.
I'm not too fond of this mem::replace dance we have to do.
rust-highfive
commented
Aug 1, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Thomasdezeeuw
commented
Aug 2, 2019
The commits have become a bit messy, I'll squash them before merging once the review is complete. |
alexcrichton
commented
Aug 2, 2019
Ok everything looks great to me, thanks! If you want to do some cleanup feel free! @bors: delegate+ |
bors
commented
Aug 2, 2019
✌️ @Thomasdezeeuw can now approve this pull request |
Thomasdezeeuw
commented
Aug 3, 2019
This is ready now, I've squashed the commits and rebased on master. |
Thomasdezeeuw
commented
Aug 5, 2019
Hope this works. @bors r+ |
bors
commented
Aug 5, 2019
📌 Commit dad56c3 has been approved by |
bors
commented
Aug 5, 2019
⌛ Testing commit dad56c3 with merge ebaa05e81052cc5f04d68880275ba073021d5f9a... |
…omasdezeeuw
Add {IoSlice, IoSliceMut}::advance
API inspired by the [`Buf::advance`](https://docs.rs/bytes/0.4.12/bytes/trait.Buf.html#tymethod.advance) method found in the [bytes](https://docs.rs/bytes) crate.
Closesrust-lang#62726.| panic!("advancing IoSliceMut beyond its length"); | ||
| } | ||
| unsafe { |
There was a problem hiding this comment.
Could we always uphold the minimum standard of having comments on all unsafe { blocks?
I think that's something you should be able to expect of something so central as the standard library.
Centril
commented
Aug 5, 2019
@bors retry rolled up. |
bors
commented
Aug 6, 2019
⌛ Testing commit dad56c3 with merge 1828a3f06b20e9c99e443d2a99fd424016735a05... |
Centril
commented
Aug 6, 2019
@bors retry yielding to r0llup. |
bors
commented
Aug 6, 2019
⌛ Testing commit dad56c3 with merge 015346c142811b881b28c5e81c1b27e59fe286d8... |
Centril
commented
Aug 6, 2019
@bors retry yielding to r0llup. |
bors
commented
Aug 6, 2019
Add {IoSlice, IoSliceMut}::advance
API inspired by the [`Buf::advance`](https://docs.rs/bytes/0.4.12/bytes/trait.Buf.html#tymethod.advance) method found in the [bytes](https://docs.rs/bytes) crate.
Closes#62726.bors
commented
Aug 6, 2019
☀️ Test successful - checks-azure |
API inspired by the
Buf::advancemethod found in the bytes crate.Closes#62726.