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.

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 Vec::drain in BufWriter #27024

Merged
merged 1 commit into from
Jul 14, 2015
Merged

Use Vec::drain in BufWriter #27024

merged 1 commit into from
Jul 14, 2015

Conversation

bluss
Copy link
Member

@bluss bluss commented Jul 13, 2015

Use Vec::drain in BufWriter

I happened past a comment that asked for functionality that we now have.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler
Copy link
Member

Does calling drain actually remove the items or is it lazy?

@bluss
Copy link
Member Author

bluss commented Jul 13, 2015

It removes all the drained items regardless if you iterate it or not. I checked the compilation of this use case back when I added drain, it should compile to the same code. (I.e. just the ptr::copy).

@bluss
Copy link
Member Author

bluss commented Jul 13, 2015

Regarding laziness, during iteration it only reads out values (ptr::read), the actual removal doesn't happen until the Drain value is dropped (ptr::copy and Vec::set_len).

@alexcrichton
Copy link
Member

@bors: r+ b9543ae360e27804747759cd4cc085ca4aaee80d

Yay!

@bors
Copy link
Contributor

bors commented Jul 14, 2015

⌛ Testing commit b9543ae with merge 181a8e5...

@bors
Copy link
Contributor

bors commented Jul 14, 2015

💔 Test failed - auto-mac-64-opt

I happened past a comment that asked for functionality that we now have.
@bluss
Copy link
Member Author

bluss commented Jul 14, 2015

Sigh, forgot to remove an import and didn't see that warning

@bluss
Copy link
Member Author

bluss commented Jul 14, 2015

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 14, 2015

📌 Commit 7b51c1c has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 14, 2015

⌛ Testing commit 7b51c1c with merge e4e9319...

bors added a commit that referenced this pull request Jul 14, 2015
Use Vec::drain in BufWriter

I happened past a comment that asked for functionality that we now have.
@bluss
Copy link
Member Author

bluss commented Jul 14, 2015

Bors come back, stop being weird 😭

@alexcrichton alexcrichton merged commit 7b51c1c into rust-lang:master Jul 14, 2015
@alexcrichton
Copy link
Member

This passed all tests except for the android builder which apparently disappeared, so I'm going to merge this manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants