search

rust-num / num-rational

Generic Rational numbers for Rust
Apache License 2.0
144 stars 51 forks source link

Smarter comparisons #49

Closed maxbla closed 5 years ago

maxbla commented 5 years ago

Ratio{numer:0, denom:1} is not equal to Ratio{numer:0, denom:2}

The code that appears to deal with this is

// With equal numerators, the denominators can be inversely compared
if self.numer == other.numer {
    let ord = self.denom.cmp(&other.denom);
    return if self.numer < T::zero() {
        ord
    } else {
        ord.reverse()
    };
}

It seems like we could easily add a case for self.numer == T::zero(), but this ties in with #8 and I don't see a need to rush a fix.

cuviper commented 5 years ago

Is there a way that you created 0/2 besides new_raw? I think all other paths should reduce to 0/1 already.

Regardless, we should fix this, just as the general comparison doesn't assume they are reduced. Would you like to send a PR? Note that using self.numer.is_zero() is slightly better to avoid construction.

maxbla commented 5 years ago

I encountered it in testing after removing a call to reduce() in AddAssign. I decided to not mess with reduce for now.

I would be happy to submit a PR for this, but I'd like to mostly finish #42 first.