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.

Sign up for GitHub

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

Implement write_all_vectored for VecDeque #136819

Closed
wants to merge 1 commit into from
Closed

Implement write_all_vectored for VecDeque #136819

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions library/std/src/io/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this is not #[inline]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the policy is to avoid #[inline] by default, but I can add it for consistency with other methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.write_vectored(bufs)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thaliaarchi thaliaarchi Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Ok(())
}

#[inline]
fn is_write_vectored(&self) -> bool {
true
Expand Down
Loading