with_prec handles negative numbers weirdly; in particular, differently from round

[colt-browning]

Long before with_scale and round were shipped, I used my own version of round based on with_prec:

fn with_accu(bd: BigDecimal, accu: i64) -> BigDecimal {
                with_accu_ref(&bd, accu)
}
 
fn with_accu_ref(bd: &BigDecimal, accu: i64) -> BigDecimal {
                let mut prec = accu + bd.digits() as i64 - bd.as_bigint_and_exponent().1;
                if prec < 0 {prec = 0};
                bd.with_prec(prec as u64)
}

It doesn’t suffer from the bug #69 and passes all test cases for round but the following four:

    #[test]
    fn test_accu_problematic() {
        let test_cases = vec![
            ("-1.555", 2, "-1.56"),
            ("-1", -1, "0"),
            ("-9999.444455556666", 10, "-9999.4444555567"),
            ("-12345678987654321.123456789", 8, "-12345678987654321.12345679"),
        ];
        for &(x, digits, y) in test_cases.iter() {
            let a = BigDecimal::from_str(x).unwrap();
            let b = BigDecimal::from_str(y).unwrap();
            assert_eq!(with_accu(a, digits), b, "test: {:?}", (x, digits, y));
        }
    }

(However, with_accu fails the test case ("7", -2, "0") in this simple implementation, so maybe this test is worth adding to the test suit for round.)

This means that with_prec and round use different logic for the rounding of negative numbers. In fact, with_prec in general handles negative numbers in an unexpected way. In particular, the following test passes:

    #[test]
    fn test_prec_pos() {
        let test_cases = vec![
            ("1.555", 3, "1.56"),
            ("1", 0, "0"),
            ("9999.444455556666", 14, "9999.4444555567"),
            ("12345678987654321.123456789", 25, "12345678987654321.12345679"),
            ("1.234", 2, "1.2"),
            ("-1.234", 1, "-1.2"),
        ];
        for &(x, prec, y) in test_cases.iter() {
            let a = BigDecimal::from_str(x).unwrap();
            let b = BigDecimal::from_str(y).unwrap();
            assert_eq!(a.with_prec(prec), b, "test: {:?}", (x, prec, y));
        }
    }

(Compare the last two test cases! This is probably not OK, and maybe deserves a separate issue.)

And every one of the following test cases fails (both the versions with correct precision and the versions with incorrect precision adjusted to counter the odd with_prec behavior with negative numbers so it at least gives the correct number of digits):

    #[test]
    fn test_prec_neg() {
        let test_cases = vec![
            ("-1.555", 2, "-1.56"),
            ("-1.555", 3, "-1.56"),
            ("-1", 0, "0"),
            ("-9999.444455556666", 14, "-9999.4444555567"),
            ("-12345678987654321.123456789", 24, "-12345678987654321.12345679"),
            ("-12345678987654321.123456789", 25, "-12345678987654321.12345679"),
        ];
        for &(x, prec, y) in test_cases.iter() {
            let a = BigDecimal::from_str(x).unwrap();
            let b = BigDecimal::from_str(y).unwrap();
            assert_eq!(a.with_prec(prec), b, "test: {:?}", (x, prec, y));
        }
    }

I didn’t put any thought in the question whether this should be fixed and if yes then how exactly, but I’m quite certain that this issue deserves being recorded here.

[colt-browning]

As of version 0.4.1, many things have changed. with_prec still rounds the negative numbers differently from (and arguably worse than) round, but the inconsistency is now much less drastic: the number of resulting digits is now the same for round and my with_prec-based with_accu. (Although with_accu now fails more tests for round.)

[akubera]

Finally going through backlog of issues here.

Yes, with_prec and with_scale round with a very old method. I can reimplement using the newer with_prec_round() and use the default rounding mode.

[akubera]

Something like this: 5cce611

[colt-browning]

That's much better, thank you.
Now this only needs tests so that this doesn't regress in future. You can use mine. Here they are (excluding the tests for my custom with_accu function):

    #[test]
    fn test_with_prec() {
        let test_cases = vec![
            ("-1.555", 3, "-1.56"),
            ( "1.555", 3,  "1.56"),
            ("-1", 0, "0"),
            ( "1", 0, "0"),
            ("-9999.444455556666", 14, "-9999.4444555567"),
            ( "9999.444455556666", 14,  "9999.4444555567"),
            ("-12345678987654321.123456789", 25, "-12345678987654321.12345679"),
            ( "12345678987654321.123456789", 25,  "12345678987654321.12345679"),
            ("-1.234", 2, "-1.2"),
            ( "1.234", 2,  "1.2"),
        ];
        for &(x, prec, y) in test_cases.iter() {
            let a = BigDecimal::from_str(x).unwrap();
            let b = BigDecimal::from_str(y).unwrap();
            assert_eq!(a.with_prec(prec), b, "test: {:?}", (x, prec, y));
        }
    }

[akubera]

Actually I did write tests but never pushed to GitHub before working on another branch.

I incorporated your test values, and made the test stricter "1.555".with_prec(3) => int_val="156", scale=2, instead of equality. Also split each test case into its own function with macros, so we can see all cases that fail, not just the first one.

trunk...fix/with_prec