Skip to content

Remove unnecessary Option from BufWriter #36336

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

Closed
wants to merge 1 commit into from
Closed

Remove unnecessary Option from BufWriter #36336

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
27 changes: 18 additions & 9 deletions src/libstd/io/buffered.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ use cmp;
use error;
use fmt;
use io::{self, DEFAULT_BUF_SIZE, Error, ErrorKind, SeekFrom};
use mem;
use memchr;
use ptr;

/// The `BufReader` struct adds buffering to any reader.
///
Expand Down Expand Up @@ -297,7 +299,7 @@ impl<R: Seek> Seek for BufReader<R> {
/// the `stream` is dropped.
#[stable(feature = "rust1", since = "1.0.0")]
pub struct BufWriter<W: Write> {
inner: Option<W>,
inner: W,
buf: Vec<u8>,
// #30888: If the inner writer panics in a call to write, we don't want to
// write the buffered data a second time in BufWriter's destructor. This
Expand Down Expand Up @@ -365,7 +367,7 @@ impl<W: Write> BufWriter<W> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn with_capacity(cap: usize, inner: W) -> BufWriter<W> {
BufWriter {
inner: Some(inner),
inner: inner,
buf: Vec::with_capacity(cap),
panicked: false,
}
Expand All @@ -377,7 +379,7 @@ impl<W: Write> BufWriter<W> {
let mut ret = Ok(());
while written < len {
self.panicked = true;
let r = self.inner.as_mut().unwrap().write(&self.buf[written..]);
let r = self.inner.write(&self.buf[written..]);
self.panicked = false;

match r {
Expand Down Expand Up @@ -412,7 +414,7 @@ impl<W: Write> BufWriter<W> {
/// let reference = buffer.get_ref();
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn get_ref(&self) -> &W { self.inner.as_ref().unwrap() }
pub fn get_ref(&self) -> &W { &self.inner }

/// Gets a mutable reference to the underlying writer.
///
Expand All @@ -430,7 +432,7 @@ impl<W: Write> BufWriter<W> {
/// let reference = buffer.get_mut();
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn get_mut(&mut self) -> &mut W { self.inner.as_mut().unwrap() }
pub fn get_mut(&mut self) -> &mut W { &mut self.inner }

/// Unwraps this `BufWriter`, returning the underlying writer.
///
Expand All @@ -451,7 +453,14 @@ impl<W: Write> BufWriter<W> {
pub fn into_inner(mut self) -> Result<W, IntoInnerError<BufWriter<W>>> {
match self.flush_buf() {
Err(e) => Err(IntoInnerError(self, e)),
Ok(()) => Ok(self.inner.take().unwrap())
Ok(()) => unsafe {
// use `ptr::read` to extract the inner writer and circumvent
// our `Drop` implementation
mem::replace(&mut self.buf, Vec::new());
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I find using mem::replace here pretty confusing - in my opinion ptr::reading the buffer (maybe with a comment why) would convey the intent much better (dropping the buffer), especially since we rely on the fact that Vec::new() never allocates (which is document, but still).

Copy link
Member

@bluss bluss Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe std::ptr::drop_in_place(&mut self.buf) would be the clearest then (it does what it says), but I still want to favour safe code when it should be equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I understand why you prefer mem::replace. Both are fine with me I guess, although I think a short comment that buf is being dropped here probably wouldn't go amiss.

Copy link
Contributor

Choose a reason for hiding this comment

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

drop_in_place won't work anymore with the removal of drop flags, right?

Copy link
Member

Choose a reason for hiding this comment

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

Sure it works

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I conflated it with read_and_drop.

On Thu, Sep 8, 2016 at 4:37 PM, bluss [email protected] wrote:

In src/libstd/io/buffered.rs
#36336 (comment):

@@ -451,7 +453,14 @@ impl<W: Write> BufWriter {
pub fn into_inner(mut self) -> Result<W, IntoInnerError<BufWriter>> {
match self.flush_buf() {
Err(e) => Err(IntoInnerError(self, e)),

  •        Ok(()) => Ok(self.inner.take().unwrap())

  •        Ok(()) => unsafe {

  •            // use `ptr::read` to extract the inner writer and circumvent

  •            // our `Drop` implementation

  •            mem::replace(&mut self.buf, Vec::new());

Sure it works

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/36336/files/ea72b7efc9f8a7fc2234264cbc6fc2241f66cd52#r78084035,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3nxeWhhJT2ZkrrBbzzvR9e2an8Wp3ks5qoHHzgaJpZM4J3ebB
.

let inner = ptr::read(&self.inner);
mem::forget(self);
Ok(inner)
},
}
}
}
Expand All @@ -464,7 +473,7 @@ impl<W: Write> Write for BufWriter<W> {
}
if buf.len() >= self.buf.capacity() {
self.panicked = true;
let r = self.inner.as_mut().unwrap().write(buf);
let r = self.inner.write(buf);
self.panicked = false;
r
} else {
Expand All @@ -481,7 +490,7 @@ impl<W: Write> Write for BufWriter<W> {
impl<W: Write> fmt::Debug for BufWriter<W> where W: fmt::Debug {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt.debug_struct("BufWriter")
.field("writer", &self.inner.as_ref().unwrap())
.field("writer", &self.inner)
.field("buffer", &format_args!("{}/{}", self.buf.len(), self.buf.capacity()))
.finish()
}
Expand All @@ -500,7 +509,7 @@ impl<W: Write + Seek> Seek for BufWriter<W> {
#[stable(feature = "rust1", since = "1.0.0")]
impl<W: Write> Drop for BufWriter<W> {
fn drop(&mut self) {
if self.inner.is_some() && !self.panicked {
if !self.panicked {
// dtors should not panic, so we ignore a failed flush
let _r = self.flush_buf();
}
Expand Down