Implement write_all_vectored for VecDeque
#136819
Conversation
|
r? @Noratrieb rustbot has assigned @Noratrieb. Use |
rustbot
added
S-waiting-on-review
| @@ -565,6 +565,11 @@ impl<A: Allocator> Write for VecDeque<u8, A> { | |||
| Ok(len) | |||
| } | |||
|
|
|||
| fn write_all_vectored(&mut self, bufs: &mut [IoSlice<'_>]) -> io::Result<()> { | |||
There was a problem hiding this comment.
Is there a reason this is not #[inline]?
There was a problem hiding this comment.
I think that the policy is to avoid #[inline] by default, but I can add it for consistency with other methods.
There was a problem hiding this comment.
Almost all the other methods in this file have #[inline], so I'd think these should have it too. The only two exceptions seem to have been mistakes and I'm adding them in a soon-to-appear PR.
| @@ -565,6 +565,11 @@ impl<A: Allocator> Write for VecDeque<u8, A> { | |||
| Ok(len) | |||
| } | |||
|
|
|||
| fn write_all_vectored(&mut self, bufs: &mut [IoSlice<'_>]) -> io::Result<()> { | |||
| self.write_vectored(bufs)?; | |||
There was a problem hiding this comment.
Can you please write a comment above that says that this is OK because write_vectored for VecDeque always writes all data? It's not immediately obvious.
That said, I'm not convinced that this method here should be overriden. The default implementation will simply call write_vectored as well with some stuff around but I think it's perfectly fine to leave the default here. I don't think we win anything from overriding it.
There was a problem hiding this comment.
Or alternatively, move the body of write_vectored into write_all_vectored and call in the other direction.
Edit: This won't work, because write_all_vectored doesn't return the length.
There was a problem hiding this comment.
In #137051, I inspected the codegen for default implementation of write_all_vectored for io::Empty, and it has a lot of slicing logic that can't be optimized out. The same would apply here. I think it's useful to specialize it.
|
FYI, this was superseded by #137107, which just merged. |
cc #136756