Skip to content

Implement FixedSizeArray for all fixed size arrays #28088

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

Merged
merged 1 commit into from
Sep 1, 2015

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Aug 29, 2015

No description provided.

@rust-highfive
Copy link
Contributor

r? @pcwalton

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

@tbu-
Copy link
Contributor Author

tbu- commented Aug 29, 2015

Code from this reddit post by @eddyb.

@bluss
Copy link
Member

bluss commented Aug 29, 2015

This is quite amazing. Ideally the element type should be an associated type, but I know the impl isn't allowed that way.

impl<T, A: Unsize<[T]>> FixedSizeArray<T> for A {
#[inline]
fn as_slice(&self) -> &[T] {
unsafe { &*(self as *const A as *const [T]) }
Copy link
Member

Choose a reason for hiding this comment

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

This can be just self as &[T] or in fact just self, and let coercion do the rest?

@huonw
Copy link
Member

huonw commented Aug 29, 2015

(It'd be great to have a more descriptive commit message or maybe some comments: I can definitely see people looking at this and wondering what's going on in future.)

@eddyb
Copy link
Member

eddyb commented Aug 29, 2015

How well does it work with coherence?
Can you, e.g. implement Clone for [T; N] via A: Unsize<[T]> without breaking anything?

@tbu-
Copy link
Contributor Author

tbu- commented Aug 29, 2015

@eddyb No, it unfortunately doesn't work with coherence at all.

@eddyb
Copy link
Member

eddyb commented Aug 29, 2015

@tbu- I tried making it smarter at some point: it should be possible to teach coherence that impl<T> Clone Foo<T> cannot conflict with impl<T, A: Unsize<[T]>> Clone for A due to Foo<_>: Unsize<[_]> never holding, but it's non-trivial and quite hacky.
cc @nikomatsakis @arielb1 Maybe there's a better way to handle this.

@alexcrichton
Copy link
Member

Using this for other blanket impls would make me a little uneasy, but this is an unstable trait anyway so I think this is fine. Could you squash the commits together and expand the first commit message like @huonw mentioned? Otherwise looks good to me!

Do so by using the fact that fixed size arrays (like `[u8; 8]` can be coerced
to slices `&[u8]`, this is expressed through the trait `Unsize<[T]>` that all
fixed size arrays implement.
@tbu- tbu- force-pushed the pr_fixed_size_array branch from 5174841 to 4d2709d Compare August 31, 2015 08:57
@tbu-
Copy link
Contributor Author

tbu- commented Aug 31, 2015

@alexcrichton Squashed and expanded the first commit message.

@bluss
Copy link
Member

bluss commented Aug 31, 2015

Yeah I don't believe this is what we'd want in the future (it's not how trait support for fixed size arrays would look if we had designed for it).

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 31, 2015

Does anybody actually use FixedSizeArray in its current form?
It couldn't be used for impls like

impl<U: Clone> Clone for T where T: FixedSizeArray<U> {...}

like it was initially planned due to coherence restrictions, and without it it's almost useless, probably I should have killed it in the same PR that introduced it.

@tbu-
Copy link
Contributor Author

tbu- commented Aug 31, 2015

@petrochenkov The trait is useful for abstractions over arrays, such as stack-allocated vectors, hybrid vectors, etc.

@alexcrichton
Copy link
Member

@bors: r+ 4d2709d

@bors
Copy link
Collaborator

bors commented Aug 31, 2015

⌛ Testing commit 4d2709d with merge d9a55d0...

@bors
Copy link
Collaborator

bors commented Aug 31, 2015

💔 Test failed - auto-linux-64-nopt-t

@alexcrichton
Copy link
Member

@bors:retry

On Mon, Aug 31, 2015 at 2:34 PM, bors [email protected] wrote:

[image: 💔] Test failed - auto-linux-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/6223


Reply to this email directly or view it on GitHub
#28088 (comment).

@tbu-
Copy link
Contributor Author

tbu- commented Sep 1, 2015

@alexcrichton I believe there's a space missing after the colon in @bors: retry, this pull request isn't in approved state whereas #28115 is after @bors: retry.

@bluss
Copy link
Member

bluss commented Sep 1, 2015

@bors retry

@bors
Copy link
Collaborator

bors commented Sep 1, 2015

⌛ Testing commit 4d2709d with merge 8f8b129...

@bors
Copy link
Collaborator

bors commented Sep 1, 2015

💔 Test failed - auto-win-gnu-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Sep 1, 2015 at 4:58 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/1252


Reply to this email directly or view it on GitHub
#28088 (comment).

@bors
Copy link
Collaborator

bors commented Sep 1, 2015

⌛ Testing commit 4d2709d with merge 5c7c5bb...

@bors bors merged commit 4d2709d into rust-lang:master Sep 1, 2015
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.

9 participants