-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Switch libcore array implementations to const generics. #60466
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
05d9148
to
6067107
Compare
There are still issues with array handling for consts generics, so I don't expect this to work quite yet. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Yes, and it will also be quite a while before the bootstrapping compiler can use const generics :) |
Even when we have working const generics on nightly we cannot actually land the impls because they would commit us to const generics on stable. As such, I think this PR will end up being closed soon. |
I don't think that's true. We could use const generics unstably in rustc indefinitely to improve Rust's handling of arrays. |
Elaborate? |
Using const generics in rustc, but disallowing them in user code allows user code to take advantage of impls on arrays whose lengths are greater than 32, without making any other features of const generics visible. |
@varkor That still commits you to the underlying quantification over values (but specific to |
In theory, we could generate these impls with a macro for every value of a |
These particular impls are needed and wanted frequently enough that even if we never landed const generics I'd be heavily in support of an implementation via a custom compiler hack. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
} | ||
|
||
#[stable(since = "1.4.0", feature = "array_default")] | ||
impl<T, const N: usize> Default for [T; N] where T: Default { |
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 current Default
impl for [T; 0]
doesn't have a T: Default
bound so this would be a breaking change.
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.
In the short term, i think we can put the original definition back. In the long term, how would we solve this? N > 0
or T: !Default
or some overlapping specialization?
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.
If there was a good way to do this we could remove the bounds on the other [T; 0]
impls as well: #52246.
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.
Seems like an obvious candidate for specialization, though unfortunately I'm not aware of any existing efforts towards const-specialization. This is in the "minimal trivially sound" category, though, so again I'd personally be inclined to accept whatever minor hack we need to make it "just work".
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'm surprised that the unconstrained impl Default for [T; 0]
even exists given #52246.
Is it some early pre-1.0 addition that was overlooked during stabilization?
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.
@est31 you don't think the pattern from @rodrimati1992 would work for Serde?
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.
@dtolnay it could possibly work. Not sure yet what const generics will allow in their initial version and what they don't. If I put @rodrimati1992 's code into the playpen I get "identifiers may currently not be used for const generics" errors on the true
and false
expressions. If I replace them with {0u8==0u8}
and {0u8!=0u8}
, I get tons of ICEs (link).
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.
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.
Still ICE's as of rustc 1.37.0-nightly (3ade426ed 2019-05-30)
. I've filed a bug at #61383
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.
Now there is a compilation error: #61935
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
0024746
to
6871fe2
Compare
Rebased to master to pick up #53645. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
52a3741
to
3e8c156
Compare
🎉 Experiment
|
The crater analysis looks good. Performance is also improved with this change. Const generics as an entire feature still have a number of issues, but there don't seem to be any issues with replacing the array implementation. Now it's up to the teams to decide what to do here: #61415. |
☔ The latest upstream changes (presumably #61672) made this pull request unmergeable. Please resolve the merge conflicts. |
3e8c156
to
0138ad4
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Rebased the commits. This error disappeared for a while, but come out again on newest master. But the diagnostics might imply something wierd is going on: it says I can repro similar error with similar code. but i'm not sure whether array is actually defined this way, or is something special:
|
r? @Centril Per T-Libs and T-Lang decisions in #61415 (comment) and #61415 (comment) respectively, we would be inclined to accept @petrochenkov idea in #61415 (comment) provided that there are:
@crlf0710 Feel free to either a) turn this PR into that, b) close this PR and file a new one for that ^--. |
Sure, i will turn this PR into that in the next few days. |
Oops, I made a PR for this in #62435 without noticing this one. |
Switch libcore array implementations to const generics.