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

Use const generics for array impls [part 1] #62435

Merged
merged 1 commit into from
Jul 7, 2019

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Jul 6, 2019

Part 1 of #61415, following the plan in #61415 (comment)

Found a way that works 🙃

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2019
@@ -264,6 +267,269 @@ macro_rules! array_impls {
}
}

#[cfg(not(bootstrap))]
mod impls_using_const_generics {
Copy link
Member Author

Choose a reason for hiding this comment

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

These are separate because I don't want to rely on the bootstrap compiler's const generics.

@scottmcm
Copy link
Member Author

scottmcm commented Jul 6, 2019

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned withoutboats Jul 6, 2019
@scottmcm scottmcm added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 6, 2019
@scottmcm

This comment has been minimized.

@scottmcm
Copy link
Member Author

scottmcm commented Jul 6, 2019

Shrinks the primitive array docs from 88 pages to 7 pages
image

@scottmcm scottmcm changed the title Use const generics for array impls Use const generics for array impls [part 1] Jul 6, 2019
@tanriol
Copy link
Contributor

tanriol commented Jul 6, 2019

IMHO, AllowedArrayLength is a slightly confusing name for the documentation purposes. How about something like FullySupportedArrayLength?

@@ -4,13 +4,14 @@ warning: the feature `const_generics` is incomplete and may cause the compiler t
LL | #![feature(const_generics)]
| ^^^^^^^^^^^^^^

error[E0277]: `[T; _]` doesn't implement `std::fmt::Debug`
error[E0277]: arrays only have std trait implementations for lengths 0..=32
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

src/libcore/array.rs Outdated Show resolved Hide resolved
src/libcore/array.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

IMHO, AllowedArrayLength is a slightly confusing name for the documentation purposes. How about something like FullySupportedArrayLength?

IMO, "Fully Supported" opens up questions whereas "Allowed" closes them, which is helpful.


We'll also want a perf run for this so... @bors try

@bors
Copy link
Contributor

bors commented Jul 6, 2019

⌛ Trying commit 5b2120be7d086c48316ea816925496fd0dfe1852 with merge a32e9743bda96ce085688c3e5819c6bb7d7c7dd3...

src/libcore/array.rs Outdated Show resolved Hide resolved
@shepmaster
Copy link
Member

There’s no secret tricks we can use to have the docs say the straightforward version, is there?

where
    N <= 32,

@bjorn3
Copy link
Member

bjorn3 commented Jul 6, 2019

where
    N <= 32,

That could make people think that that is real syntax.

Maybe call AllowedArrayLength ArrayLengthAtMost32 instead?

@bors
Copy link
Contributor

bors commented Jul 6, 2019

☀️ Try build successful - checks-azure, checks-travis
Build commit: a32e9743bda96ce085688c3e5819c6bb7d7c7dd3

@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

@rust-timer build a32e9743bda96ce085688c3e5819c6bb7d7c7dd3

@rust-timer
Copy link
Collaborator

Success: Queued a32e9743bda96ce085688c3e5819c6bb7d7c7dd3 with parent 254f201, comparison URL.

@petrochenkov
Copy link
Contributor

@scottmcm
Could you compare the size of libcore before and after the change, in addition to the perf run?

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit a32e9743bda96ce085688c3e5819c6bb7d7c7dd3, comparison URL.

@Centril
Copy link
Contributor

Centril commented Jul 6, 2019

Perf looks promising; @bors rollup=never

@c410-f3r
Copy link
Contributor

c410-f3r commented Jul 6, 2019

Since this PR is part 1, does the following PRs include a feature gate to enable implementations > 32 behind a feature gate?

@est31
Copy link
Member

est31 commented Jul 6, 2019

@c410-f3r that wouldn't work I think. In general, impls are insta-stable. That won't prevent anyone from removing the check and building libstd themselves though.

@scottmcm
Copy link
Member Author

scottmcm commented Jul 7, 2019

Squashed & removed the ping against @​centril

@bors r=centril

@bors
Copy link
Contributor

bors commented Jul 7, 2019

📌 Commit d6a9793 has been approved by centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2019
@bors
Copy link
Contributor

bors commented Jul 7, 2019

⌛ Testing commit d6a9793 with merge 6e310f2...

bors added a commit that referenced this pull request Jul 7, 2019
Use const generics for array impls [part 1]

Part 1 of #61415, following the plan in #61415 (comment)

Found a way that works 🙃
@bors
Copy link
Contributor

bors commented Jul 7, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: centril
Pushing 6e310f2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2019
@bors bors merged commit d6a9793 into rust-lang:master Jul 7, 2019
@scottmcm scottmcm deleted the constrained-array-impls branch July 8, 2019 01:00
#[unstable(feature = "const_generic_impls_guard", issue = "0",
reason = "will never be stable, just a temporary step until const generics are stable")]
#[cfg(not(bootstrap))]
pub trait LengthAtMost32 {}
Copy link
Member

@varkor varkor Jul 12, 2019

Choose a reason for hiding this comment

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

Is it not possible to make this invisible outside std? It'd be better if users couldn't do:

use std::array::LengthAtMost32;

Copy link
Member

Choose a reason for hiding this comment

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

The trait is unstable. Users can't do it unless they opt into instability.

Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that it's user-facing at all, even unstably. It's not a big problem though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's good that it shows up in the impls, so that they don't look like they work for all N. And if it's going to be visible in the rustdoc there, I think it's better that the bound be clickable so people can see what it is.

@RalfJung
Copy link
Member

This PR broke Miri executing array code. I think I will need some help for this. We are getting an error somewhere in this function, likely in this line:

ConstValue::Param(_) => return err!(TooGeneric), // FIXME(oli-obk): try to monomorphize

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 23, 2019

[T; N]: LengthAtMost32,

Heh, this is the same "trick" packed_simd uses to constrain the array argument of the Simd<T> type, where T can only be an [E; N] type of particular Es and Ns.

@Centril Centril mentioned this pull request Jul 24, 2019
5 tasks
LukasKalbertodt added a commit to LukasKalbertodt/rust that referenced this pull request Jul 25, 2019
In PR rust-lang#62435 ("Use const generics for array impls [part 1]") the old
macro-based implementations were not removed but still used with
`cfg(bootstrap)` since the bootstrap compiler had some problems with
const generics at the time. This does not seem to be the case anymore,
so there is no reason to keep the old code.
Centril added a commit to Centril/rust that referenced this pull request Jul 25, 2019
…ootstrap-cfg, r=Mark-Simulacrum

Remove `cfg(bootstrap)` code for array implementations

In rust-lang#62435 ("Use const generics for array impls [part 1]") the old macro-based implementations were not removed but still used with `cfg(bootstrap)` since the bootstrap compiler had some problems with const generics at the time. This does not seem to be the case anymore, so there is no reason to keep the old code.

Unfortunately, the diff is pretty ugly because much of the code was indented by one level before. The change is pretty trivial, though.

PS: I did not run the full test suite locally. There are 40°C outside and 31°C inside my room. I don't want my notebook to melt. I hope that CI is green.

r? @scottmcm
bors added a commit that referenced this pull request Jul 28, 2019
In which we constantly improve the Vec(Deque) array PartialEq impls

Use the same approach as in #62435 as sanctioned by #61415 (comment).

r? @scottmcm
Centril added a commit to Centril/rust that referenced this pull request Jul 28, 2019
…ttmcm

In which we constantly improve the Vec(Deque) array PartialEq impls

Use the same approach as in rust-lang#62435 as sanctioned by rust-lang#61415 (comment).

r? @scottmcm
Centril added a commit to Centril/rust that referenced this pull request Jul 28, 2019
…ttmcm

In which we constantly improve the Vec(Deque) array PartialEq impls

Use the same approach as in rust-lang#62435 as sanctioned by rust-lang#61415 (comment).

r? @scottmcm
bors added a commit to rust-lang/miri that referenced this pull request Aug 20, 2019
test arrray try_from (interesting const generic usage)

Currently fails, see rust-lang/rust#62435 (comment).

Blocked on rust-lang/rust#62790.
@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.