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

Implement From<[T; ..]> for (T, ..) and From<(T, ..)> for [T; ..] #30736

Closed
wants to merge 1 commit into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jan 6, 2016

This adds:

impl<T> From<[T; 3]> for (T, T, T) { .. }
impl<T> From<(T, T, T)> for [T; 3] { .. }

And same for all other arrays and tuples dimensions, up to 32 elements for tuple->array and up to 12 elements for array->tuple (similar to the other trait implementations on arrays and tuples).

Note that the first one requires Clone, because otherwise I couldn't manage to make it compile. Fixed.

If the changes are good, this still needs some tests and an issue number.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ollie27
Copy link
Member

ollie27 commented Jan 6, 2016

I think it makes sense to only implement this for tuples up to size 12 in both directions in order to keep it symmetrical.

@tomaka
Copy link
Contributor Author

tomaka commented Jan 6, 2016

I also noticed that the order of the elements may be wrong.

Could I get some additional feedback on the nature of the changes? And if it's approved, I'll clean up the PR.

@retep998
Copy link
Member

retep998 commented Jan 6, 2016

I think something like this would be very useful. Shame about that Clone requirement though.

@apasel422
Copy link
Contributor

It's possible to implement this without the Clone bound using ptr::read and mem::forget.

EDIT: Another option is slice patterns (though they are unstable):

let [a, b, c] = [1, 2, 3];
(a, b, c)

@tomaka
Copy link
Contributor Author

tomaka commented Jan 6, 2016

Another option is slice patterns (though they are unstable):

Doesn't work either: http://is.gd/Kmwznm

But yeah I could use ptr::read.

@arielb1
Copy link
Contributor

arielb1 commented Jan 6, 2016

@tomaka

Then go the ptr way.

@apasel422
Copy link
Contributor

@tomaka This seems to work:

fn foo<T>([a, b, c]: [T; 3]) -> (T, T, T) {
    (a, b, c)
}

@bluss
Copy link
Member

bluss commented Jan 6, 2016

rustc has something related that I'd love to have. Could be AsRef impls. tuple_slice.rs

@tomaka
Copy link
Contributor Author

tomaka commented Jan 6, 2016

@bluss That's really nice, but I guess that the problem is that the internal representation of tuples may change in the future.

Anyway, this PR should now be good to go except for the issue number.

@ollie27
Copy link
Member

ollie27 commented Jan 6, 2016

I still don't think tuple->array conversions should be implemented to tuples longer than 12 as nothing else is implemented for tuples that long.

@bluss
Copy link
Member

bluss commented Jan 7, 2016

@tomaka Right. Given the rustc trait, I was hoping we could solidify the repr.

@huonw huonw added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 7, 2016
@huonw
Copy link
Member

huonw commented Jan 7, 2016

Stability doesn't work for trait implementations, so these will be automatically stable if they land.

(Nominating for libs team discussion.)

#[stable(since = "1.4.0", feature = "array_default")]
impl<T> Default for [T; $n] where T: Default {
fn default() -> [T; $n] {
[$t::default(), $($ts::default()),*]
}
}
array_impl_default!{($n - 1), $($ts)*}

#[unstable(feature = "array_from_tuple", reason = "recently added", issue = "0")]
Copy link
Member

Choose a reason for hiding this comment

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

These'll be insta-stable unfortunately so could these be #[stable] instead of #[unstable]?

@alexcrichton
Copy link
Member

I agree with @huonw that we'll likely want to chat about this in the libs triage. I'm personally a little wary about adding these sorts of impls as it seems there is no end to the various sorts of conversions that could be added.

@tomaka
Copy link
Contributor Author

tomaka commented Jan 11, 2016

The motivation behind this change is that there is no consensus on whether a geometrical vector or geometrical point should be represented as [f32; 3] or (f32, f32, f32).
Sometimes the former is more practical, sometimes it's the latter.

@alexcrichton
Copy link
Member

The libs team discussed this in triage yesterday and the conclusion was to close this for now. While we typically have these sorts of conversions for arrays it's normally done with the intention of replacing it with type-level generic integers one day. A feature such as this, however, would require a relationship between variadic generics, tuples, and type-level integers which is likely much farther out.

As a result we concluded that it should be fine to have small conversion functions in-crate for these now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants