Special-cased performance improvement for Iterator::sum on Range<u*> and RangeInclusive<u*>

[vcfxb]

Rendered

[Lokathor]

Speaking Procedurally: I'm wondering if this even needs an RFC. I think T-libs can just accept a PR that makes this improvement, since it's only a change to the quality of implementation, not a change to the API contract.

[vcfxb]

Okay sounds good! I'll make a PR and close this RFC.

[scottmcm]

Agreed -- it could be reverted at any time, so it doesn't need an RFC.

[vcfxb]

Running into an issue with implementation regarding the overlap between the unstable Step trait and Iterator -- currently I cannot trivially special-case Iterator or Iterator::sum because the signature for the RangeInclusive iterator implementation is

impl<A: Step> Iterator for ops::RangeInclusive<A> {
    type Item = A;

and all the unsized integer types implement Step.

What are the future plans for Step? It seems to have been stalled for a long time, and at this point does not clearly provide much in terms of advantages over the existing iterator implementation using StepBy.

[vcfxb]

This may actually not be an issue -- just reading now about trait specialization and the default keyword.

[vcfxb]

Nope I can't figure this out -- rust-lang/rust#55436 is stopping me from getting any of this stuff to build properly.

[tgross35]

@alfriadox open a PR (to rust-lang/rust) with your change and link it here, you can leave this RFC PR closed.

You shouldn't need to add any attributes to do this, it doesn't need feature gating - are you trying to add a stability attribute to the sum function?

[vcfxb]

I was trying to add it to the impl Iterator for Range<u*>, which requires the specialization feature because u* also implements the unstable Step trait and there's an existing impl for Iterator on Range<T> where T: Step. This would be a lot easier honestly if we made u* not implement Step but at that point we might as well get rid of Step entirely, which doesn't make sense. Most of this comes down to specialization working properly

[teor2345]

Just a question about the scope of this change: why aren't we specialising for signed integer types as well?

Here's an example of guide text for signed ranges:

We can generalize this to work for a signed range by taking the sum of the positive range, then subtracting the sum of the absolute value of the negative range. Then the sums of both ranges are calculated using the positive sum function.

For example, to do `sum(-4..=10)` we would do 
`sum(1..=10) - sum(1..=4)`: `10*11/2 - 4*5/2 == 55-10 == 45 == -4-3-2-1+0+1+2+3+4+5+6+7+8+9+10 == sum(-4..=10)`.

Handling ranges that include the minimum signed value requires converting the absolute value to the unsigned type of the same width. As an alternative, these ranges can be special-cased, because there are only two signed sums starting at the minimum value where the naive sum doesn't underflow: i128::MIN itself, and i128::MIN + (i128::MIN - 1).

[teor2345]

This formula also preserves the same overflow behaviour, as long as the positive and negative sums are cast back to the original type before being added together. (And the negative sun is negated before the cast.)

[tgross35]

I was trying to add it to the impl Iterator for Range<u*>, which requires the specialization feature because u* also implements the unstable Step trait and there's an existing impl for Iterator on Range<T> where T: Step. This would be a lot easier honestly if we made u* not implement Step but at that point we might as well get rid of Step entirely, which doesn't make sense. Most of this comes down to specialization working properly

Just make a PR with what you have, even if it doesn't work (just mark it as a draft), and link it here. We can help you get there, but it's easier when we can see the implementation and the errors.

[Noratrieb]

I'm closing this, as it's not an RFC anymore. Do open a PR as others have said already, and feel free to ask questions on the PR (or on zulip).