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

BufWriter's inner does not need to be an Option #36319

Closed
kestein opened this issue Sep 7, 2016 · 6 comments
Closed

BufWriter's inner does not need to be an Option #36319

kestein opened this issue Sep 7, 2016 · 6 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@kestein
Copy link

kestein commented Sep 7, 2016

In the implementation of BufWriter the option type for the inner writer does not seem to be necessary. There is no way to initialize the inner writer to be None and the value is unwrap()'d anywhere the value of it is needed.

@TimNN
Copy link
Contributor

TimNN commented Sep 7, 2016

The Option is necessary to allow for an into_inner implementation in the presence of a Drop implementation.

(The Option is set to None during the take in into_inner).

@apasel422
Copy link
Contributor

@TimNN into_inner could be rewritten to use ptr::read and mem::forget.

@TimNN
Copy link
Contributor

TimNN commented Sep 7, 2016

@apasel422: Yeah, you're right, so let me rephrase: An Option is necessary for a safe into_inner implementation (in the presence of a Drop implementation).

And personally I don't think the unsafe is worth it here, since whatever cost is associated with the Option is probably negligible when compared to the cost of the actual I/O (and, IMO, the less unsafe the better).

@hanna-kruppe
Copy link
Contributor

A benchmark would be good, but if there is any measurable gain to be had, then this is exactly (one of the reasons) why unsafe exists. It might be sad that this pattern can't be expressed safely, but that's a grievance with the language, not a mark against the pattern, modulo the practical concerns of how sure one is about the correctness of the implementation. But I'm not super worried about the safety since the unsafe code would be rather small, simple, and contained.

@brson brson added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Sep 7, 2016
@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@frewsxcv
Copy link
Member

Anyone have ideas for a benchmark for this?

@Mark-Simulacrum
Copy link
Member

I'm going to go ahead and close as this is a pretty ugly change based on the couple PRs that tried to fix this and the additional complexity isn't really worth it (no benchmarks are able to show significant wins).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants