Fixed an issue with NumericRanges until/to Long.MinValue's length #10328
Conversation
| @@ -402,7 +402,7 @@ object NumericRange { | |||
| val startside = num.sign(start) | |||
| val endside = num.sign(end) | |||
| num.toInt{ | |||
| if (num.gteq(num.times(startside, endside), zero)) { | |||
| if (num.gt(num.times(startside, endside), zero)) { | |||
There was a problem hiding this comment.
[0, Long.MinValue] by any negative step is handled here, while it's definitely not safe to subtract 0 with MinValue.
There was a problem hiding this comment.
The condition is now a very slow way to write startside == endside. So one could use that instead. But the reason to include zero is to make the most usual range, which is [0, n), to be easy to calculate.
Can we just drop startside and endside entirely and instead compute num.lt(start, zero) == num.lt(end, zero) (i.e. either both negative or both non-negative)? This will only admit Int.MinValue when the range is no bigger than -1 to Int.MinValue, which is okay to subtract.
SethTisue
added
the
library:collections
|
let's see if we can interest @Ichoran in scrutinizing this one too... |
There was a problem hiding this comment.
I am I'm not entirely sure that this is correct, but I haven't tried very hard to check because of a different issue. The change makes the comments no longer apply: you don't necessarily have three pieces, or if you do, they're not the pieces it says. In order to change this code, it's very helpful to have comments that reflect what is going on. Furthermore, the comments that were added assume Long, but we don't know that it's a Long. We only know that it doesn't fit in Int.
So you could carefully update the documentation, explaining the different possible states.
But I recommend adding a third case, instead. The first case stays the same (but use equal signs as the test, which is equivalent to but faster than the multiplication and test against zero). The second, one-endpoint-is-zero case does not compute the difference at all, instead first it divides the nonzero value by step and then subtracts if the nonzero value was the start; then you can check the sign and if it's wrong, it was going to be out of bounds anyway so just throw an exception. If it's okay, use the same logic as in the both-same-sign case. The third case stays at it is, with the documentation matching that case.
Anyway, either way I think some changes are in order.
|
@AminMal interested in returning to this? |
@SethTisue Yeah for sure, I'll make sure to improve the comments and update the PR accordingly. |
|
@Ichoran Sorry for the late response, and thanks for the review!
Right now I'm coming up with an update to this PR which makes it "more readable", it'd be great if you could take a look at it again, to see if you agree with the new documentations on it. Also if you think any of the alternatives is a better way to solve this issue, please let me know. |
|
@SethTisue Is it okay for me to mark this PR as "ready to review"? |
|
@AminMal absolutely, go ahead! |
| @@ -402,7 +402,7 @@ object NumericRange { | |||
| val startside = num.sign(start) | |||
| val endside = num.sign(end) | |||
| num.toInt{ | |||
| if (num.gteq(num.times(startside, endside), zero)) { | |||
| if (num.gt(num.times(startside, endside), zero)) { | |||
There was a problem hiding this comment.
The condition is now a very slow way to write startside == endside. So one could use that instead. But the reason to include zero is to make the most usual range, which is [0, n), to be easy to calculate.
Can we just drop startside and endside entirely and instead compute num.lt(start, zero) == num.lt(end, zero) (i.e. either both negative or both non-negative)? This will only admit Int.MinValue when the range is no bigger than -1 to Int.MinValue, which is okay to subtract.
|
@AminMal interested in returning to this...? |
|
closing for inactivity — we can reopen it if @AminMal reappears, or a volunteer could submit a new PR based on this one |
Fixes an issue with the evaluation of length on NumericRanges going onto
Long.MinValue.Fixes scala/bug#12716