Skip to content
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.

Sign up for GitHub

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

Fix panic when trying to compare big numbers #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+111 −41
Open

Fix panic when trying to compare big numbers #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 111 additions & 41 deletions src/cmp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,98 @@
use crate::iter::{iterate_lexical, iterate_lexical_only_alnum};
use core::cmp::Ordering;

macro_rules! cmp_ascii_digits {
(first_digits($lhs:ident, $rhs:ident), iterators($iter1:ident, $iter2:ident)) => {
let mut n1 = ascii_to_u64($lhs);
let mut n2 = ascii_to_u64($rhs);
loop {
match (
$iter1.peek().copied().filter(|c| c.is_ascii_digit()),
$iter2.peek().copied().filter(|c| c.is_ascii_digit()),
) {
(Some(lhs), Some(rhs)) => {
n1 = n1 * 10 + ascii_to_u64(lhs);
n2 = n2 * 10 + ascii_to_u64(rhs);
let _ = $iter1.next();
let _ = $iter2.next();
}
(Some(_), None) => return Ordering::Greater,
(None, Some(_)) => return Ordering::Less,
(None, None) => {
if n1 != n2 {
return n1.cmp(&n2);
use core::{
cmp::Ordering,
iter::Peekable,
};

fn cmp_ascii_digits(lhs: &mut Peekable<impl Iterator<Item=char>>, rhs: &mut Peekable<impl Iterator<Item=char>>) -> Option<Ordering> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[derive(PartialEq)]
enum Origin {
Lhs,
Rhs,
}

// 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 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c: char,
origin: Origin,
}

impl core::ops::Deref for NonDigit {
type Target = char;

fn deref(&self) -> &Self::Target {
&self.c
}
}

impl NonDigit {
#[allow(dead_code)]

fn is_lhs(&self) -> bool {
self.origin == Origin::Lhs
}

fn is_rhs(&self) -> bool {
self.origin == Origin::Rhs
}
}

fn ok_if_ascii_digit(c: char) -> Result<char, char> {
Some(c).filter(char::is_ascii_digit).ok_or(c)
}

let mut current_cmp = None;
loop {
match (lhs.peek(), rhs.peek()) {
(Some(&a), Some(&b)) => {
let non_digit = match (ok_if_ascii_digit(a), ok_if_ascii_digit(b)) {
(Ok(a), Ok(b)) => {
// Only update current_cmp if the current comparison is yet undecided.
// current_cmp is returned later when at least one iterator has hit a non-digit.
if current_cmp.is_none() || current_cmp == Some(Ordering::Equal) {
current_cmp = Some(a.cmp(&b));
}
None
},
(Err(c), Ok(_)) => Some(NonDigit{ c, origin: Origin::Lhs }),
(Ok(_), Err(c)) => Some(NonDigit{ c, origin: Origin::Rhs }),
(Err(_), Err(_)) => break current_cmp,
};

// Advance underlying iterators, since we only peek and break early if no iterator
// has any digits left, keeping these characters in the iterators for the caller to
// deal with in case current_cmp.is_none() or current_cmp == Some(Ordering::Equal).
let _ = lhs.next();
let _ = rhs.next();

// Return the appropriate ordering of a number versus non-digit characters.
if let Some(c) = non_digit {
let mut ord = if current_cmp.is_none() && c.is_alphanumeric() {
Ordering::Greater
} else {
break;
Ordering::Less
};
if c.is_rhs() {
ord = ord.reverse();
}
break Some(ord);
}
}
(Some(_), None) => {
let _ = lhs.next();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break Some(Ordering::Greater);
}
(None, Some(_)) => {
let _ = rhs.next();
break Some(Ordering::Less);
}
(None, None) => {
break current_cmp;
}
}
};
}

#[inline]
fn ascii_to_u64(c: char) -> u64 {
(c as u64) - (b'0' as u64)
}
}

#[inline]
Expand Down Expand Up @@ -100,11 +160,13 @@ pub fn natural_lexical_cmp(s1: &str, s2: &str) -> Ordering {
let mut iter2 = iterate_lexical(s2).peekable();

loop {
match cmp_ascii_digits(&mut iter1, &mut iter2) {
None | Some(Ordering::Equal) => (),
Some(result) => return result,
}
match (iter1.next(), iter2.next()) {
(Some(lhs), Some(rhs)) => {
if lhs.is_ascii_digit() && rhs.is_ascii_digit() {
cmp_ascii_digits!(first_digits(lhs, rhs), iterators(iter1, iter2));
} else if lhs != rhs {
if lhs != rhs {
return ret_ordering(lhs, rhs);
}
}
Expand All @@ -123,11 +185,13 @@ pub fn natural_lexical_only_alnum_cmp(s1: &str, s2: &str) -> Ordering {
let mut iter2 = iterate_lexical_only_alnum(s2).peekable();

loop {
match cmp_ascii_digits(&mut iter1, &mut iter2) {
None | Some(Ordering::Equal) => (),
Some(result) => return result,
}
match (iter1.next(), iter2.next()) {
(Some(lhs), Some(rhs)) => {
if lhs.is_ascii_digit() && rhs.is_ascii_digit() {
cmp_ascii_digits!(first_digits(lhs, rhs), iterators(iter1, iter2));
} else if lhs != rhs {
if lhs != rhs {
return lhs.cmp(&rhs);
}
}
Expand All @@ -146,11 +210,13 @@ pub fn natural_cmp(s1: &str, s2: &str) -> Ordering {
let mut iter2 = s2.chars().peekable();

loop {
match cmp_ascii_digits(&mut iter1, &mut iter2) {
None | Some(Ordering::Equal) => (),
Some(result) => return result,
}
match (iter1.next(), iter2.next()) {
(Some(lhs), Some(rhs)) => {
if lhs.is_ascii_digit() && rhs.is_ascii_digit() {
cmp_ascii_digits!(first_digits(lhs, rhs), iterators(iter1, iter2));
} else if lhs != rhs {
if lhs != rhs {
return lhs.cmp(&rhs);
}
}
Expand All @@ -169,11 +235,13 @@ pub fn natural_only_alnum_cmp(s1: &str, s2: &str) -> Ordering {
let mut iter2 = s2.chars().filter(|c| c.is_alphanumeric()).peekable();

loop {
match cmp_ascii_digits(&mut iter1, &mut iter2) {
None | Some(Ordering::Equal) => (),
Some(result) => return result,
}
match (iter1.next(), iter2.next()) {
(Some(lhs), Some(rhs)) => {
if lhs.is_ascii_digit() && rhs.is_ascii_digit() {
cmp_ascii_digits!(first_digits(lhs, rhs), iterators(iter1, iter2));
} else if lhs != rhs {
if lhs != rhs {
return lhs.cmp(&rhs);
}
}
Expand Down Expand Up @@ -329,6 +397,8 @@ mod tests {
ordered("T-27", "Ŧ-5");
ordered("T-5", "Ŧ-27");
ordered("T-5", "Ŧ-5");

ordered("00000000000000000000", "18446744073709551616");
}

#[test]
Expand Down