-
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
use checked write in LineWriter
example
#51628
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
src/libstd/io/buffered.rs
Outdated
/// for &byte in road_not_taken.iter() { | ||
/// file.write(&[byte]).unwrap(); | ||
/// } | ||
/// file.write_all(road_not_taken)?; |
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.
The example is explicitly writing each byte individually to note how the buffering provided by LineWriter helps things. If we were going to write the whole message out at once, there's no reason to use the LineWriter at all.
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.
It's still line buffering behind the scenes, so I don't think that it's totally wrong. I'm certainly open to suggestions on how to improve the entire example, though. Maybe we could write some bytes, and demonstrate that they aren't written to disk until a newline is written?
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.
That seems reasonable.
Ping from triage, @withoutboats : This PR needs your review! |
This isn't ready for review, I plan to rewrite the example per sfackler's review. |
The original example didn't check the return value of `write()`, didn't flush the writer, and didn't properly demonstrate the buffering. Fixes rust-lang#51621.
@sfackler @withoutboats This is ready for another review. |
src/libstd/io/buffered.rs
Outdated
/// | ||
/// // No bytes are written until a newline is encountered (or | ||
/// // the internal buffer is filled). | ||
/// assert_eq!(fs::read_to_string("poem.txt")?.as_bytes(), b""); |
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.
I think fs::read_to_string(...)?.as_bytes()
can just be fs::read(...)?
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.
Alternatively this can be assert_eq!(fs::read_to_string("poem.txt")?, "")
.
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.
Fixed.
Ping from triage, @withoutboats: This PR is waiting for your review. |
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.
This looks great, thanks for this 🎉
@bors r+ rollup |
📌 Commit c12a757 has been approved by |
use checked write in `LineWriter` example The example was wrong because it didn't check the return value of `write()`, and it didn't flush the buffer before comparing the contents of the file. Fixes rust-lang#51621.
Rollup of 13 pull requests Successful merges: - #51628 (use checked write in `LineWriter` example) - #52116 (Handle array manually in str case conversion methods) - #52218 (Amend option.take examples) - #52418 (Do not use desugared ident when suggesting adding a type) - #52439 (Revert some changes from #51917 to fix custom libdir) - #52455 (Fix doc comment: use `?` instead of `.unwrap()`) - #52458 (rustc: Fix a suggestion for the `proc_macro` feature) - #52464 (Allow clippy to be installed with make install) - #52472 (rustc: Enable `use_extern_macros` in 2018 edition) - #52477 (Clarify short-circuiting behvaior of Iterator::zip.) - #52480 (Cleanup #24958) - #52487 (Don't build twice the sanitizers on Linux) - #52510 (rustdoc: remove FIXME about macro redirects) Failed merges: r? @ghost
The example was wrong because it didn't check the return value of
write()
, and it didn't flush the buffer before comparing the contentsof the file.
Fixes #51621.