Fix panic when trying to compare big numbers #2
Conversation
The previous version tried to convert numeric strings to u64 to compare the values numerically. This results in a panic if the number exceeds the bounds of u64. To fix this, numbers are now compared digit-by-digit instead. Fixes surrealdb#1.
| iter::Peekable, | ||
| }; | ||
|
|
||
| fn cmp_ascii_digits(lhs: &mut Peekable<impl Iterator<Item=char>>, rhs: &mut Peekable<impl Iterator<Item=char>>) -> Option<Ordering> { |
There was a problem hiding this comment.
I'm not big on macros that change control-flow outside their scope, like cmd_ascii_digits!() did via return. I've opted to replace it with a regular function, but that's personal preference. If you liked the macro with return for the caller better, I can change it back.
| } | ||
| } | ||
| (Some(_), None) => { | ||
| let _ = lhs.next(); |
There was a problem hiding this comment.
In practice, callers don't continue using the iterators when this returns an ordering != Ordering::Equal, so consuming the ascii digit in the non-exhausted iterator is not actually used for anything.
I just didn't feel comfortable to make this assumption in the function implementation, but I can remove it if you feel this is unnecessary.
| // The loop below iterates through both iterators at once and handles ascii digits for comparison. | ||
| // If one iterator runs out of ascii digits, it is stored in this struct together with the | ||
| // information where it originated from. | ||
| struct NonDigit { |
There was a problem hiding this comment.
This may be a bit overengineered for what can also be tracked with a (char, bool), but I think/hope this makes the use more clear.
The previous version tried to convert numeric strings to u64 to compare the values numerically.
This results in a panic if the number exceeds the bounds of u64. To fix this, numbers are now compared digit-by-digit instead.
Fixes #1.
This PR is pretty much identical to Aloso/lexical-sort#11. I'm copying the PR comments from there.