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

std: Allow ?Sized parameters in std::io::copy #27531

Merged
merged 1 commit into from
Aug 10, 2015
Merged

Conversation

bluss
Copy link
Member

@bluss bluss commented Aug 5, 2015

std: Allow ?Sized parameters in std::io::copy

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@bluss
Copy link
Member Author

bluss commented Aug 5, 2015

r? @alexcrichton I think this is a sensible generalization

@alexcrichton
Copy link
Member

Oh interesting! I thought we had already done this... We may actually want to take this in the opposite direction and instead change the signature to:

fn copy<R: Read, W: Write>(r: R, w: W) -> io::Result<u64>;

That's the generalization we've taken with a bunch of AsRef<Path> and AsRef<OsStr> parameters throughout std::{fs, path}. It primarily cleans up the signature (no need for ?Sized) and you can still pass in &mut Read as an instance of Read itself.

@tomaka
Copy link
Contributor

tomaka commented Aug 5, 2015

@alexcrichton That could be a breaking change.

Currently the only way to call copy if you have a &mut Read and a &mut Write is to pass a &mut &mut Read and a &mut &mut Write. Changing the signature would break this code.

@alexcrichton
Copy link
Member

Yeah it's in theory a breaking change because the meaning of io::copy::<R, W> changes (no more implicit references), but in that case &mut &mut Read should still implement Read so code will continue to compile.

I'd want to run this through crater to make sure there's no regressions we know of, and if there is then we can stick with ?Sized for now.

@tomaka
Copy link
Contributor

tomaka commented Aug 5, 2015

in that case &mut &mut Read should still implement Read so code will continue to compile

Oh, right.

@bluss
Copy link
Member Author

bluss commented Aug 5, 2015

@alexcrichton I'd love to change it that way instead — it would be more consistent with other I/O API too.

@bluss bluss changed the title std: Allow ?Sized parameters in std::io::copy Generalize std::io::copy Aug 5, 2015
@bluss
Copy link
Member Author

bluss commented Aug 5, 2015

Updated, has [breaking-change] tag in commit log since we try to track everything that's technically breaking.

This change is what I want to do too, I just didn't realize it would be possible. But all is fine, except for the type parameter change.

@alexcrichton
Copy link
Member

I've kicked off a crater build to evaluate the impact

@alexcrichton alexcrichton added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 5, 2015
@alexcrichton
Copy link
Member

Hm, looks like we may not be able to do this, the were three regressions, although I'm not sure why at least one of them hit an error...

@bluss
Copy link
Member Author

bluss commented Aug 5, 2015

Oh that was unexpected, but it's good we tried that out in crater.

All the cases seem to be that &mut T values are consumed by copy instead of reborrowed.

Back to the original plan, to just add ?Sized...

@bluss bluss changed the title Generalize std::io::copy std: Allow ?Sized parameters in std::io::copy Aug 5, 2015
@bluss
Copy link
Member Author

bluss commented Aug 5, 2015

PR is back to the original plan..

@@ -45,7 +45,8 @@ use io::{self, Read, Write, ErrorKind, BufRead};
/// # }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn copy<R: Read, W: Write>(reader: &mut R, writer: &mut W) -> io::Result<u64> {
pub fn copy<R: ?Sized + Read, W: ?Sized + Write>(reader: &mut R, writer: &mut W)
-> io::Result<u64> {
Copy link
Member Author

Choose a reason for hiding this comment

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

If you have time, what's our actual best style for wrapping function signatures?

libstd seems to want this:

A:

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

B:

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

I usually do this personally, dislike { hanging so far to the right

C:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Grepping around is dizzying. There are a few different styles active!

Copy link
Member

Choose a reason for hiding this comment

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

I would personally vote for the ideal style of:

pub fn copy<R, W>(reader: &mut R, writer: &mut W) -> io::Result<u64>
    where R: Read + ?Sized, W: Write + ?Sized 
{ 
    /* ... */
}

Unfortunately ?Sized doesn't work in a where clause right now so that's not possible. Overall though I'd go with option (C). I do agree though that we're not consistent throughout the codebase on what style of where clauses we have.

@alexcrichton
Copy link
Member

Could you also add a test that calls this with trait objects?

@alexcrichton
Copy link
Member

cc @gankro, mysterious travis failures (spurious)

@Gankra
Copy link
Contributor

Gankra commented Aug 6, 2015

@alexcrichton That is mind boggling. 0.o

@bluss
Copy link
Member Author

bluss commented Aug 6, 2015

PR updated with style & a test

@bluss
Copy link
Member Author

bluss commented Aug 6, 2015

Thanks @tomaka for bringing this bug to attention


#[test]
fn copy_copies() {
let mut r = repeat(4);
Copy link
Member Author

Choose a reason for hiding this comment

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

apparently I don't know how repeat works. Fixing this..

std::io::copy did not allow passing trait objects directly (only with an
extra &mut wrapping).
@bluss
Copy link
Member Author

bluss commented Aug 10, 2015

Updated, test fixed, r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ 664fdbe

@alexcrichton alexcrichton removed the I-needs-decision Issue: In need of a decision. label Aug 10, 2015
@bors
Copy link
Contributor

bors commented Aug 10, 2015

⌛ Testing commit 664fdbe with merge 75383ea...

bors added a commit that referenced this pull request Aug 10, 2015
std: Allow ?Sized parameters in std::io::copy
@bors bors merged commit 664fdbe into rust-lang:master Aug 10, 2015
@bluss bluss deleted the io-copy-dst branch August 11, 2015 00:10
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants