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

Change copy_into/copy_buf_into to free functions for consistency with the standard library #1948

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 5, 2019

No description provided.

@ghost
Copy link

ghost commented Nov 5, 2019

The signature of std::io::copy is:

pub fn copy<R: ?Sized, W: ?Sized>(reader: &mut R, writer: &mut W) -> Result<u64>
where
    R: Read,
    W: Write;

Is there a reason why in this PR we take ownership of reader, i.e. take R instead of &mut R?

@Nemo157
Copy link
Member

Nemo157 commented Nov 5, 2019

It was required to make delegating the implementation to copy_buf_into possible. It also more closely follows the API guideline C-RW-VALUE:

Generic reader/writer functions take R: Read and W: Write by value

Given that copy exhausts the provided reader there's no reason to allow access to it afterwards, and users can still borrow it anyway if they want.

EDIT: Here's the previous discussion when the API was changed.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 5, 2019

The reason is explained in this comment in #1674.

we want to wrap the reader in a BufReader to allow the reuse, but then we don't have anywhere with ownership of that BufReader when we pass the reference into CopyBufInto::new (unless we make CopyInto a self-referential struct and only create the inner CopyBufInto when we're first polled, but I'd prefer to not deal with manually implemented self-referential structs).

(Also, this is why tokio is considering changing the definition of AsyncBufRead)

EDIT: I didn't notice that @Nemo157 commented.

@ghost
Copy link

ghost commented Nov 5, 2019

@Nemo157 If API guidelines suggest taking Read and Write arguments by value, why not take writer here by value, too (W instead of &mut W)?

I ran into similar issues before: async-rs/async-std#365

Personally, I'd prefer if we always took Read and Write arguments by value (like API guidelines suggest), even in the standard library. We could even make this change in standard library's APIs without breaking them, AFAICT.

@cramertj
Copy link
Member

cramertj commented Nov 5, 2019

@stjepang unlike reader, writer is not exhausted by this operation.

@cramertj cramertj merged commit 023b8cf into rust-lang:master Nov 5, 2019
@taiki-e taiki-e deleted the copy branch November 5, 2019 17:39
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.

3 participants