Skip to content

Implement FixedSizeArray for all fixed size arrays#28088

Merged
bors merged 1 commit into
rust-lang:masterfrom
tbu-:pr_fixed_size_array
Sep 1, 2015
Merged

Implement FixedSizeArray for all fixed size arrays#28088
bors merged 1 commit into
rust-lang:masterfrom
tbu-:pr_fixed_size_array

Conversation

@tbu-

@tbu- tbu- commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

No description provided.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @pcwalton

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

@tbu-

tbu- commented Aug 29, 2015

Copy link
Copy Markdown
Contributor Author

Code from this reddit post by @eddyb.

@bluss

bluss commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

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

Comment thread src/libcore/array.rs Outdated

Copy link
Copy Markdown
Contributor

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

huonw commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

(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

eddyb commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

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

@tbu-

tbu- commented Aug 29, 2015

Copy link
Copy Markdown
Contributor Author

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

@eddyb

eddyb commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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-

tbu- commented Aug 31, 2015

Copy link
Copy Markdown
Contributor Author

@alexcrichton Squashed and expanded the first commit message.

@bluss

bluss commented Aug 31, 2015

Copy link
Copy Markdown
Contributor

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

petrochenkov commented Aug 31, 2015

Copy link
Copy Markdown
Contributor

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-

tbu- commented Aug 31, 2015

Copy link
Copy Markdown
Contributor Author

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

@alexcrichton

Copy link
Copy Markdown
Member

@bors: r+ 4d2709d

@bors

bors commented Aug 31, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4d2709d with merge d9a55d0...

@bors

bors commented Aug 31, 2015

Copy link
Copy Markdown
Collaborator

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

@alexcrichton

Copy link
Copy Markdown
Member

@bors:retry

On Mon, Aug 31, 2015 at 2:34 PM, bors notifications@github.com 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-

tbu- commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

@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

bluss commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

@bors retry

@bors

bors commented Sep 1, 2015

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Sep 1, 2015

Copy link
Copy Markdown
Collaborator

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

@alexcrichton

Copy link
Copy Markdown
Member

@bors: retry

On Tue, Sep 1, 2015 at 4:58 AM, bors notifications@github.com 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

bors commented Sep 1, 2015

Copy link
Copy Markdown
Collaborator

⌛ 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