mhogrefe / malachite

An arbitrary-precision arithmetic library for Rust.
GNU Lesser General Public License v3.0
448 stars 17 forks source link

Inconsistency between code and comments #45

Closed YichiZhang0613 closed 3 months ago

YichiZhang0613 commented 3 months ago

I found some inconsistency between code and comments. In malachite-master/malachite-nz/src/natural/random/mod.rs, the comments "Panics ... or if $a > b$"indicate that there is a constraint a <= b. However, the code uses assert!(a < b); to check it.

/// # Panics
/// Panics if `mean_stripe_denominator` is zero, if `mean_stripe_numerator <=
/// mean_stripe_denominator`, or if $a > b$.
#[inline]
pub fn striped_random_natural_range(seed: Seed, a: Natural, b: Natural, mean_stripe_numerator: u64, mean_stripe_denominator: u64,) -> StripedRandomNaturalInclusiveRange {
    assert!(a < b);

In malachite-master/malachite-q/src/exhaustive/mod.rs, the comments "Panics ... or if $a < b$"indicate that there is a constraint a >= b. However, the code uses assert!(a <= b); to check it.

/// # Panics
/// Panics if `d` is zero or if $a < b$.
pub fn exhaustive_rationals_with_denominator_inclusive_range(
    d: Natural,
    a: Rational,
    b: Rational,
) -> RationalsWithDenominator<ExhaustiveIntegerRange> {
    assert_ne!(d, 0u32);
    assert!(a <= b);

In malachite-master/malachite-nz/src/natural/arithmetic/div.rs, the comments indicate the comment other != 0 while the code doesn't check it. There are a lot of similar cases in this file and malachite-master/malachite-nz/src/integer/arithmetic/div.rs and malachite-master/malachite-nz/src/natural/arithmetic/mod_op.rs

/// # Panics
/// Panics if `other` is zero.
fn div(mut self, other: Natural) -> Natural {
        self /= other;
        self
    }
mhogrefe commented 3 months ago

Thanks for catching the inconsistencies in the a and b assertions. I fixed them as part of v0.4.11.

That div implementation example does panic; it forwards to the div_assign implementation, which has an explicit panic. There is a unit test for this situation here.

Likewise, the Integer division and modulo functions forward to the Natural division and modulo functions, which panic.

YichiZhang0613 commented 3 months ago

I found 3 more In malachite-master/malachite-base/src/num/basic/floats.rs, it might be more appropriate to use exponent instead of self in comments(Panics if exponent is less than [MIN_EXPONENT]).

/// # Panics
/// Panics if `self` is less than [`MIN_EXPONENT`](PrimitiveFloat::MIN_EXPONENT) or greater than
/// [`MAX_EXPONENT`](PrimitiveFloat::MAX_EXPONENT).
fn max_precision_for_sci_exponent(exponent: i64) -> u64 {
        assert!(exponent >= Self::MIN_EXPONENT);
        assert!(exponent <= Self::MAX_EXPONENT);

In malachite-master/malachite-base/src/num/random/geometric.rs, the comment indicates that a < b and the code checks a <= b

/// # Panics
/// Panics if $a \geq b$, if `um_numerator` or `um_denominator` are zero, if their ratio is less
/// than or equal to $a$, or if they are too large and manipulating them leads to arithmetic
/// overflow.
pub fn geometric_random_unsigned_inclusive_range<T: PrimitiveUnsigned>(seed: Seed, a: T, b: T, um_numerator: u64, um_denominator: u64,) -> GeometricRandomNaturalValues<T> {
    assert!(a <= b, "a must be less than or equal to b. a: {a}, b: {b}");
    geometric_random_natural_values_range(seed, a, b, um_numerator, um_denominator)
}

In malachite-master/malachite-base/src/num/random/striped.rs, the comment indicates that a <= b and the code checks a < b

/// # Panics
/// Panics if `mean_stripe_denominator` is zero, if `mean_stripe_numerator <=
/// mean_stripe_denominator`, or if $a > b$.
pub fn striped_random_signed_range< U: PrimitiveUnsigned + WrappingFrom<S>, S: PrimitiveSigned + WrappingFrom<U>,>(seed: Seed, a: S, b: S, mean_stripe_numerator: u64, mean_stripe_denominator: u64,) -> StripedRandomSignedInclusiveRange<U, S> {
    assert!(a < b);