Fix Range#size for unsigned edge cases#14978
Fix Range#size for unsigned edge cases#14978straight-shoota merged 2 commits intocrystal-lang:masterfrom
Range#size for unsigned edge cases#14978Conversation
straight-shoota
added
kind:bug
| n < 0 ? 0 : n.to_i32 | ||
| return 0 if e < b | ||
|
|
||
| diff = (e &- b).to_i32.abs |
There was a problem hiding this comment.
This can't be right.. how can it be safe to just not check for overflows 🤔
We should double-check it, I'll probably do so today
There was a problem hiding this comment.
Good catch. e < b only excludes the obvious case. 🤦
We can still end up with a difference that doesn't fit into E (e.g. (Int8::MIN..1i8).size). But it would fit into Int32.
I suppose we could use non-wrapping arithmetics and cast e to Int32 if it's a smaller type. That should probably cover every constellation where the result fits into Int32.
There was a problem hiding this comment.
There are so many edge cases to take into account 🤯
I'm wondering if we should collect a set of
for testing Range methods with optimized implementations for Int (i.e. the ones mentioned in #13648: size, sum, step.sum, map; maybe also sample).
4 participants
Resolves part of #13648