-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Let Range#step
delegate to begin.step
#10209
Let Range#step
delegate to begin.step
#10209
Conversation
|
||
# Overrides `Enumerable#sum` to use more performant implementation on integer | ||
# ranges. | ||
def sum(initial) |
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 there an existing spec for this?
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.
Yes, I just copied this method from Range::StepIterator
because the sum specs in range_spec
happen to call this method. Especially these specs runs very long without it:
crystal/spec/std/range_spec.cr
Lines 110 to 111 in cb09b0e
(BigInt.new("1")..BigInt.new("1 000 000 000")).sum.should eq BigInt.new("500 000 000 500 000 000") | |
(BigInt.new("1")..BigInt.new("1 000 000 000")).step(2).sum.should eq BigInt.new("250 000 000 000 000 000") |
src/number.cr
Outdated
# Overrides `Enumerable#sum` to use more performant implementation on integer | ||
# ranges. | ||
def sum(initial) | ||
super if @reached_end |
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.
Shouldn't this return?
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.
Haha, yeah. That's just copied 1:1 from Range::StepIterator
😆
Can't wait to step through Time! But what's the default step there? |
None. I mentioned this in the OP. Stepping through non-discrete types like |
TLDR; This lets
(1.0..5.0).step
work.Resolves #9339
Precursers: #10203, #10130
Range#step
previously was based on calling#succ
to get the next value in the iteration. This works for discrete types like integers which define#succ
. Floating point numbers don't have a clear successor, soFloat#succ
doesn't exist andRange(Float64, Float64)#step
wouldn't work.This patch lets
Range(B, E)#step
delegate toB#step
(if defined), so the behaviour can be entirely implemented by the value's type. The expected interface is that ofNumber#step
which is also extended in this patch by anexclusive
argument to handle exclusive ranges.The implementation of
Number#step
is very generic based on simple algebra methods and works for non-number types as well. So a follow up would extract it for re-usability (an example would beTime
as per #9327).Note: Non-discrete types don't have an inherent step size. For stepping a
Time
range for example, you would need to specifiy the step size explicitly.Range#step
uses1
as default value for the step size, but if the step implementation doesn't accept integer step size, it errors at compile time. The error message doesn't point exactly to the call toRange#step
, but I guess that's acceptable.The previous
#succ
-based implementation is retained as an alternative ifbegin
doesn't respond to#step
. This is still used forString
ranges. I'm not sure if we want to keep this alternative. It's not really necessary,Range
can just expect every stepable type to implement#step
. But this is a more complex discussion, because#succ
and#pred
methods are currently the defining interface of value types for many methods onRange
.So, for now I would just add this as an enhancement. The diff in
range.cr
is actually pretty minimal, it's best viewed with whitespace option.