implement const Iterator for Range - #156216
Conversation
|
Another issue I noticed is that |
This comment has been minimized.
This comment has been minimized.
|
Does this run into #148200 for types that implement non-const |
This comment has been minimized.
This comment has been minimized.
|
@theemathas |
|
Maybe there's actually not a problem with specialization here? I suppose it's kind of fine to call a function that requires |
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| impl<A: Step> DoubleEndedIterator for ops::Range<A> { | ||
| #[rustc_const_unstable(feature = "const_iter", issue = "92476")] | ||
| impl<A: [const] Step + [const] Destruct> const DoubleEndedIterator for ops::Range<A> { |
There was a problem hiding this comment.
Is this change needed for the MVP? should at least be a separate commit unless the rest of this commit requires it
There was a problem hiding this comment.
It is, in fact, required since last() is implemented using next_back(), so it needs to be constified too.
|
I feel iffy on introducing additional const impls that use specialization without us fixing specialization first |
|
This no longer builds since bc2112e. Probably related to a88521a. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like genuine ICE for me, I may take a look at it tomorrow |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I'll rebase once I get to PC, but it is still blocked on const min specialization resolution (see discussion at https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23148200.3A.20min_specialization.20with.20const.20traits.20can.20call.20non-.E2.80.A6/with/614859233 ) I do think (and had a feeling that others agree) that this feature, i.e. allowing |
|
To be clear, I mostly said what I did because I hadn't fully read the PR and didn't want to make any guarantees. Taking a second look, nothing looks particularly egregious, although will probably want a second opinion from wg-const-traits on what some of these tests were intended to do since it looks like they were relying on iterator for ranges not being implemented. |
This comment has been minimized.
This comment has been minimized.
b39ec5d to
6d992a8
Compare
This comment has been minimized.
This comment has been minimized.
|
If we're okay with specializing on |
|
r? libs |
|
Returning from RustConf and finally looking at this: I think that the constification of the various methods on (Feel free to In terms of the rest, I'm a bit worried about the UI tests being affected, since I don't think the newer versions of the tests are testing what we want to test. The codegen test, however, looks fine, since it's an expected equivalence created by switching to a for loop. I'm going to mark this as blocked until a separate PR for the iterator changes, but like I said, would be happy to review that separately if you submit it, and you can specify me for review. It's mostly just so that we can separate these out for loads of reasons, including potential perf checks + to get a bit more scrutiny on what exactly is being affected by that versus the range changes. @rustbot blocked |
| let mut x = 0; | ||
|
|
||
| for i in 0..4 { | ||
| //~^ ERROR: cannot use `for` |
There was a problem hiding this comment.
This seems to clearly test loops in const context
|
|
||
| const _: () = { | ||
| for _ in 0..5 {} | ||
| //~^ ERROR cannot use `for` |
There was a problem hiding this comment.
this also appears to test specifically for for in const, though it duplicates
|
I've opened #162734 for |
Constify more Iterator and DoubleEndedIterator functions required for Range Extracted from rust-lang#156216 Consitfies functions required for `const impl Iterator for Range`. r? clarfonthey
This comment has been minimized.
This comment has been minimized.
6d992a8 to
dac6393
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
View all comments
This allows the use of
for i in i..ninconst.This I believe is interesting enough to be justified under #155816
r? @oli-obk