-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -264,6 +267,269 @@ macro_rules! array_impls { | |||
} | |||
} | |||
|
|||
#[cfg(not(bootstrap))] | |||
mod impls_using_const_generics { |
There was a problem hiding this comment.
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.
r? @varkor |
This comment has been minimized.
This comment has been minimized.
IMHO, |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
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 |
⌛ Trying commit 5b2120be7d086c48316ea816925496fd0dfe1852 with merge a32e9743bda96ce085688c3e5819c6bb7d7c7dd3... |
There’s no secret tricks we can use to have the docs say the straightforward version, is there?
|
That could make people think that that is real syntax. Maybe call |
☀️ Try build successful - checks-azure, checks-travis |
@rust-timer build a32e9743bda96ce085688c3e5819c6bb7d7c7dd3 |
Success: Queued a32e9743bda96ce085688c3e5819c6bb7d7c7dd3 with parent 254f201, comparison URL. |
@scottmcm |
Finished benchmarking try commit a32e9743bda96ce085688c3e5819c6bb7d7c7dd3, comparison URL. |
Perf looks promising; @bors rollup=never |
Since this PR is part 1, does the following PRs include a feature gate to enable implementations |
@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. |
Squashed & removed the ping against @centril @bors r=centril |
📌 Commit d6a9793 has been approved by |
Use const generics for array impls [part 1] Part 1 of #61415, following the plan in #61415 (comment) Found a way that works 🙃
☀️ Test successful - checks-azure, checks-travis, status-appveyor |
#[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 {} |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 impl
s, 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.
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: rust/src/librustc_mir/interpret/operand.rs Line 534 in a9c7feb
|
Heh, this is the same "trick" |
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.
…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
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
…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
…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
test arrray try_from (interesting const generic usage) Currently fails, see rust-lang/rust#62435 (comment). Blocked on rust-lang/rust#62790.
Part 1 of #61415, following the plan in #61415 (comment)
Found a way that works 🙃