Skip to content

Round with negative decimalDigits returns 0.12 for 1234 (and 0 for 1500) when the value has no fractional part #105

Description

@matt-edmondson

What's wrong

Round(int decimalDigits) (PreciseNumber/PreciseNumber.cs:548) accepts negative decimalDigits without validation. It computes the new exponent like this:

int newExponent = newSignificand.IsZero ? 0 : Exponent - int.CopySign(droppedDigits, Exponent);

This is only correct when Exponent < 0, where it gives Exponent + droppedDigits. When Exponent >= 0, CountDecimalDigits() returns 0, so any negative decimalDigits takes the rounding branch. Then:

  • Exponent == 0: CopySign(d, 0) is +d, so the exponent moves the wrong way (0 - d).
  • Exponent > 0: the exponent is also moved down rather than up.
  • droppedDigits is computed from decimalDigits - 0, ignoring that the stored exponent already puts the significand's last digit Exponent places to the left of the units. As a result, too many digits are dropped.

The same call works when the value happens to have a fractional part, so the result depends on how the value is stored rather than on its value.

Failure scenario

PreciseNumber.Parse("1234.5", null).Round(-2); // 1200  (correct)
PreciseNumber.Parse("1234",   null).Round(-2); // 0.12  (expected 1200)
PreciseNumber.Parse("1230",   null).Round(-2); // 0.1   (expected 1200)
PreciseNumber.Parse("1500",   null).Round(-3); // 0     (expected 2000, half away from zero)

The static Round(PreciseNumber, int) overload at line 1722 forwards to this method and behaves the same way.

Suggested fix

Work out the number of digits to drop from the target place, and use it for every exponent sign:

long targetExponent = -(long)decimalDigits;
if (Exponent < targetExponent) {
    int dropped = (int)long.Min(targetExponent - Exponent, SignificantDigits + 1);
    BigInteger sig = DropDigitsRoundingHalfAwayFromZero(Significand, dropped);
    return sig.IsZero ? Zero : new PreciseNumber(checked(Exponent + dropped), sig);
}
return this;

If negative digits are not meant to be supported, throw ArgumentOutOfRangeException for decimalDigits < 0, as decimal.Round and Math.Round do. Either way, the current mix of silently wrong results and correct ones is not acceptable.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions