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
SI-6736 Range.contains is wrong #3437
Conversation
Related, what should
|
Regardless, the fix itself is an improvement. We should probably also actively sanity-check the value of |
I should fix last as well while I'm at it. (I wouldn't revert this change once last is fixed, though, as I got a 4-5x speedup on contains.) |
Perfect. |
Updated with a second commit to fix |
I'm confused -- why isn't |
@adriaanm - Because the range is too large, and it should be consistent with |
Then why override |
Just going for consistency. If that's not so important (or if the point of |
I'm honestly not sure which is the more consistent choice. I tend to go for minimizing WTF/method call. |
I think of a Range as a lazy collection that you can iterate over. Technically, we're not even breaking the invariant |
I think everything is surprising since there's no obvious expectation when creating something that acts as a collection but is outside the bounds of what a collection can contain. But it's not hard to get |
Removed once-used private method that was calculating ranges in error and corrected the contains method (plus improved performance).
Operations are reasonable when they don't require indexing or conversion into a collection. These include head, tail, init, last, drop, take, dropWhile, takeWhile, dropRight, takeRight, span. Tests added also to verify the new behavior.
PLS REBUILD ALL |
@Ichoran Wow, nice fixes! Having struggled with that code some time ago, I can imagine the pain you went through to fix this! Thanks a lot! |
LGTM -- excellent code, nice tests, much fewer WTF/line for users! |
SI-6736 Range.contains is wrong
soon some of these wishes will be granted |
Removed once-used private method that was calculating ranges in error and corrected the contains method (plus improved performance).