-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
Add io::Write::write_all_vectored #70612
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
I went with the original implementation I suggested, just to get some experience with it. I was thinking that maybe we could always set |
@Thomasdezeeuw Can you split the module after this PR? It's getting close to the 3000 LOC threshold. |
@Centril Sure, anything you suggest that should move to its own file? Or should I start with moving the tests? Also I only ran libcore tests locally, so I'll fix the pr and push :) |
@Thomasdezeeuw Some themes to consider:
|
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 |
Similar to io::Write::write_all but uses io::Write::write_vectored instead.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few inline comments, mostly documentation though.
Regarding the discussion in #70436: I am also not very happy about an API like this. Especially since we recommend in the docs to "treat it as if bufs
was consumed by this method". Feels wrong to me, as in Rust, we should be able to assert this on a type level somehow. But maybe it's not that bad, as this is mainly a fairly low level API anyway. And unfortunately, I don't have a great idea how to use the type system to make this better.
I personally don't like the idea of making this method implementation more complicated or slower to get around this API quirk. So I'm not quite sure what to do about this...
Also adds some more tests with different length IoSlices.
@LukasKalbertodt I've address all comments, except for the tracking issue one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny thing still!
Thanks! As I said, I don't particularly like the API. But let's get it landed, it's unstable after all. We can still change and discuss the exact API later. @bors r+ |
📌 Commit 5d09f9f has been approved by |
…albertodt Add io::Write::write_all_vectored Similar to io::Write::write_all but uses io::Write::write_vectored instead. Updates rust-lang#70436 /cc @cramertj @sfackler
…albertodt Add io::Write::write_all_vectored Similar to io::Write::write_all but uses io::Write::write_vectored instead. Updates rust-lang#70436 /cc @cramertj @sfackler
…albertodt Add io::Write::write_all_vectored Similar to io::Write::write_all but uses io::Write::write_vectored instead. Updates rust-lang#70436 /cc @cramertj @sfackler
Rollup of 5 pull requests Successful merges: - rust-lang#70612 (Add io::Write::write_all_vectored) - rust-lang#70690 (Clean up E0501 explanation) - rust-lang#70821 (expose suggestions::InferCtxtExt for clippy) - rust-lang#70839 (clean up E0506 explanation) - rust-lang#70859 (Move sanitize-inline-always test to sanitize directory) Failed merges: r? @ghost
I'm very happy to see this method added, thank you! |
Similar to io::Write::write_all but uses io::Write::write_vectored
instead.
Updates #70436
/cc @cramertj @sfackler