Fix LazyList.range [ci: last-only]
#10767
Conversation
SethTisue
added
the
library:collections
| .range(start = 0L, end = totalElements) | ||
| .foldLeft(0L) { case (acc, _) => | ||
| acc + 1L | ||
| } |
There was a problem hiding this comment.
even with inlining turned on, it's a lot to ask of a unit test. Maybe it's sufficient to test construction and head.
There was a problem hiding this comment.
Yeah I was wondering the same.
However, not sure if just asking for the head ensures there are more than Int.MaxValue elements.
So not sure if there is a smart way to test this without it being a CPU trap.
There was a problem hiding this comment.
Using a reverse range we don't need to compute a lot of elements.
Let me know what you think of the new test.
C.C. @NthPortal
|
added the magic title to avoid testing every commit |
| .range(start = 0L, end = totalElements) | ||
| .foldLeft(0L) { case (acc, _) => | ||
| acc + 1L | ||
| } |
There was a problem hiding this comment.
Yeah I was wondering the same.
However, not sure if just asking for the head ensures there are more than Int.MaxValue elements.
So not sure if there is a smart way to test this without it being a CPU trap.
Co-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>
There was a problem hiding this comment.
just two small changes; otherwise lgtm.
I agree with @som-snytt's concern about the cost of iterating over Int.MaxValue elements, though I'm not really sure what the alternative is. I will defer to others' judgement about whether or not to just skip it
…tion Co-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>
Co-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>
| } | ||
|
|
||
| private[this] def rangeImpl[A](start: A, end: A, step: A)(implicit ev: Integral[A]): State[A] = | ||
| if (ev.lt(start, end)) sCons(start, newLL(rangeImpl(ev.plus(start, step), end, step))) |
There was a problem hiding this comment.
Uhm, wait, this does not allow reversed ranges, does it? Like LazyList.range(start = 100, end = 0, step = -2)
I guess rather than check for lt we have to check for !ev.equiv(start, end)?
There was a problem hiding this comment.
Also, this made me think. This could be a good way to test this without a CPU trap. We just request the reversed range and then we can indeed ask for the head and check it is bigger than Int.MaxValue
There was a problem hiding this comment.
good point, the check is incorrect, hmm. probably need to check the sign or something as well. I'll have another look shortly
There was a problem hiding this comment.
my thought is to check the sign of step in the 3 parameter overload of range, and then pass it as a Boolean parameter (probably named positiveStep) to rangeImpl, thus checking the condition only once rather than for each element
There was a problem hiding this comment.
I just pushed an implementation that allows for reverse ranges.
Let me know what you think about it.
Since
LazyListis not strict, there is not reason to makerangedelegate throughfrom(NumericRange)which doesn't allow more thanInt.MaxValueelements.