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

#[derive(Clone)] should rely on Copy when possible #31085

Closed
SimonSapin opened this issue Jan 21, 2016 · 12 comments
Closed

#[derive(Clone)] should rely on Copy when possible #31085

SimonSapin opened this issue Jan 21, 2016 · 12 comments

Comments

@SimonSapin
Copy link
Contributor

When a type that is also Copy has #[derive(Clone)], the derived implementation should be:

    fn clone(&self) -> Self { *self }

From #27750 (comment)

As the example in #27750 (comment) shows, even when a composite type like Composite is Copy, its clone implementation may be less efficient than its Copy impl, due to copying fields individually instead of as a block.

Maybe being Copy is hard to determine exactly by the time #[derive(Clone)] needs to do its thing, but at least this could happen when both traits are in the same derive attribute? #[derive(Copy, Clone)]

@bluss
Copy link
Member

bluss commented Jan 21, 2016

A more specific example https://play.rust-lang.org/?gist=ef17b9c31e01be5f65c2&version=nightly where clone_composite is a loop and copy_composite turns into memcpy.

@sfackler
Copy link
Member

We currently just desugar #[derive(Clone, Copy)] to #[derive_Clone] #[derive_Copy] and expand those separately, but it could special case the Clone and Copy combo.

@alexcrichton
Copy link
Member

I would be quite ok specializing #[derive(Clone, Copy)], much else beyond that does seem unfortunately quite difficult :(

@SimonSapin
Copy link
Contributor Author

It’s a start, and it may even be the common case.

@retep998
Copy link
Member

I've had to use a macro to define all my structs in winapi just so I can get that efficient Copy-based Clone impl to improve compile times.

@durka
Copy link
Contributor

durka commented Feb 3, 2016

Okay, so thinking aloud about this idea of special-casing #[derive(Copy, Clone)] as #[derive_CopyClone] and making that More Magic (tm).

From the code

#[derive(Copy, Clone)]
struct S<T>(T);

fn main() { }

you get (cleaned up)

struct S<T>(T);

impl <T: Clone> Clone for S<T> {
    fn clone(&self) -> S<T> {
        match *self {
            S(ref x) => S(Clone::clone(&*x)),
        }
    }
}

impl <T: Copy> Copy for S<T> { }

fn main() { }

Now instead of the above Clone impl we would like there to be

impl<T: Clone + Copy> Clone for S<T> {
    fn clone(&self) -> S<T> {
        *self
    }
}

but now S<T>: !Clone where T: Clone + !Copy (please forgive the horrible abuse of notation). Is it possible to do this in a backwards-compatible way? Maybe it can done only for types with no generic parameters?

@durka
Copy link
Contributor

durka commented Feb 3, 2016

@bluss
Copy link
Member

bluss commented Feb 3, 2016

I think you would need specialization to supply both the where T: Clone and the where T: Copy implementations, the latter is more specific, so it should work out.

@durka
Copy link
Contributor

durka commented Feb 3, 2016

So we'll revisit this after specialization lands?

@bluss
Copy link
Member

bluss commented Feb 3, 2016

You can do it well already for the case without type parameters

@durka
Copy link
Contributor

durka commented Feb 3, 2016

Agreed.

@durka
Copy link
Contributor

durka commented Feb 4, 2016

See #31414.

bors added a commit that referenced this issue Apr 26, 2016
special-case #[derive(Copy, Clone)] with a shallow clone

If a type is Copy then its Clone implementation can be a no-op. Currently `#[derive(Clone)]` generates a deep clone anyway. This can lead to lots of code bloat.

This PR detects the case where Copy and Clone are both being derived (the general case of "is this type Copy" can't be determined by a syntax extension) and generates the shallow Clone impl. Right now this can only be done if there are no type parameters (see #31085 (comment)), but this restriction can be removed after specialization.

Fixes #31085.
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

No branches or pull requests

7 participants