Add iterator method specialisations to Range*#47180
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
src/libcore/iter/range.rs
Outdated
|
|
||
| #[inline] | ||
| fn min(self) -> Option<A> { | ||
| Some(self.start) |
There was a problem hiding this comment.
Currently min for RangeFrom will either panic with overflow or loop forever so this is a change of behaviour. As RangeFrom is fundamentally broken right now (#25708) I don't think it's a good idea to specialise methods for it just yet.
There was a problem hiding this comment.
Yeah, I was considering not adding this yet. I'll remove it, as it would be better placed in a separate change.
src/libcore/iter/range.rs
Outdated
|
|
||
| #[inline] | ||
| fn max(self) -> Option<A> { | ||
| Some(self.end) |
There was a problem hiding this comment.
Shouldn't this and last return None once the iterator is exhausted?
There was a problem hiding this comment.
Yep, it definitely should.
|
You can use either |
|
Thanks! Although now that it's been brought up, @bluss's suggestion sounds like a great way to future-proof these as well. Want to implement it via that? |
|
@bors: r+ Nice! |
|
📌 Commit 2d83343 has been approved by |
|
Something odd is going on with optimisation for |
Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.
|
💔 Test failed - status-appveyor |
Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.
|
💔 Test failed - status-appveyor |
|
⌛ Testing commit 2d83343 with merge f67c277525fa3fd4fad5624b058b411ee495e1ff... |
|
💔 Test failed - status-appveyor |
|
@bors r- I think 3 consecutive timeouts indicate there's some performance problem here (or you are really unlucky). @varkor could you investigate the "min" optimization mentioned #47180 (comment)? (We could re-r+ if it turns out there's nothing more to do) |
|
The |
|
@bors: r+ |
|
📌 Commit 919d643 has been approved by |
Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.
|
💔 Test failed - status-appveyor |
|
@bors retry |
Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.
|
☀️ Test successful - status-appveyor, status-travis |
Calling `min` on `RangeFrom` currently causes an infinite loop. Although other methods such as `max` also result in an infinite loop, it is strictly incorrect in the case of `min`. Adding a specialisation fixes this. Separated from rust-lang#47180 because this technically changes behaviour; it’s not just an optimisation, so it’s a little different.
Add specialised implementations of
maxforRange, andlast,minandmaxforRangeInclusive, all of which lead to significant advantages in the generated assembly on x86.Note that adding specialisations of
minandlastforRangeled to no benefit, and addingsumforRangeandRangeInclusiveled to type inference issues (though this is possibly still worthwhile considering the performance gain).This addresses some of the concerns in #39975.