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

Add Clone impls for extern "C" and unsafe fns #24168

Merged
merged 1 commit into from
Apr 9, 2015

Conversation

lilyball
Copy link
Contributor

@lilyball lilyball commented Apr 7, 2015

We only implemented Clone on extern "Rust" fns (for up to 8
parameters). This didn't cover extern "C" or unsafe (or
unsafe extern "C") fns, but there's no reason why they shouldn't be
cloneable as well.

The new impls are marked unstable because the existing impl for extern "Rust" fns is.

Fixes #24161.

We only implemented Clone on `extern "Rust" fn`s (for up to 8
parameters). This didn't cover `extern "C"` or `unsafe` (or `unsafe
extern "C"`) `fn`s, but there's no reason why they shouldn't be
cloneable as well.

The new impls are marked unstable because the existing impl for `extern
"Rust" fn`s is.

Fixes rust-lang#24161.
@rust-highfive
Copy link
Contributor

r? @pcwalton

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

@alexcrichton
Copy link
Member

@bors: r+ df95719

Thanks!

@richo
Copy link
Contributor

richo commented Apr 8, 2015

I noodled on this a bit more generically. There are plenty of other supported ABI's in rust, which should all get a Clone impl, right?

I'm happy to do the copypasting for the other abi's, I'm just wondering if this is going to end up accidentally combinatorial.

@Kimundi
Copy link
Member

Kimundi commented Apr 8, 2015

A way to be generic over all ABIs would be nice:

impl<const ABI: &str, ...T, U> extern ABI fn(...T) -> U { ... }

@richo
Copy link
Contributor

richo commented Apr 8, 2015

I just did it with More Macros™

@lilyball
Copy link
Contributor Author

lilyball commented Apr 8, 2015

Supporting this for other ABIs would be nice as well. I didn't do it because I don't know offhand what other ABIs are available and I didn't really want to produce a huge explosion of Clone impls.

I agree that it would be nice to be able to be generic over ABIs, but I don't know how that would really work, as the ABI isn't a value but is in fact a syntactic element (even though it uses quotes).

@richo
Copy link
Contributor

richo commented Apr 8, 2015

tbc, I'm +1 on this change. I'll PR my thing in the next few days, and then we can bikeshed on that PR?

@Kimundi
Copy link
Member

Kimundi commented Apr 8, 2015

@kballard Well it would probably involve adding values to the typesystem and redefining the syntax of a function type to take a string value at that position - nothing easily done without a bunch of RFCs post 1.0 though :).

@alexcrichton
Copy link
Member

I would rather not add bindings to all ABIs that we support for now. With specialization we will be able to day impl<T: Copy> Clone for T which will solve many of these problems. For now this is otherwise just implementing Clone for the most common function parameters found in structures that want to use #[derive(Clone)]

@alexcrichton alexcrichton assigned alexcrichton and unassigned pcwalton Apr 8, 2015
@bors
Copy link
Collaborator

bors commented Apr 9, 2015

⌛ Testing commit df95719 with merge 0e5e669...

bors added a commit that referenced this pull request Apr 9, 2015
…crichton

We only implemented Clone on `extern "Rust" fn`s (for up to 8
parameters). This didn't cover `extern "C"` or `unsafe` (or
`unsafe extern "C"`) `fn`s, but there's no reason why they shouldn't be
cloneable as well.

The new impls are marked unstable because the existing impl for `extern
"Rust" fn`s is.

Fixes #24161.
@bors bors merged commit df95719 into rust-lang:master Apr 9, 2015
@lilyball lilyball deleted the clone-for-extern-c-unsafe-fns branch April 11, 2015 22: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.

extern "C" functions don't implement Clone
7 participants