Repository navigation
fix: parse decimal byte counts without floating-point rounding - #202
Open
sudhakar810 wants to merge 2 commits into
Open
sudhakar810 wants to merge 2 commits into
sudhakar810 wants to merge 2 commits into
Conversation
Fractional inputs were converted with `(v * unit) as u64`, which truncates an `f64` product. Most decimal fractions are not exactly representable in binary, so the product often lands just below the intended integer and loses a byte: "2.01 KB" parsed as 2009 and "2.05 GB" as 2049999999. Compute the result with exact integer arithmetic instead, folding the fractional digits right to left so each step can be floored without changing the final value. This also extends the exactness fix for integers at or above 2^53 to inputs with a fractional part, and keeps the existing saturating behaviour on overflow.
Assert the digits-only precondition of `mul_decimal` in debug builds, and pad the generated fraction in the quickcheck property with leading zeros so inputs like "1.05" are exercised. The expected value is now computed as `whole * factor + floor(frac * factor / scale)` so it stays within `u128` for the longer fractions.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parsing a fractional size multiplies an f64 by the unit factor and truncates with as u64. Most decimal fractions aren't exactly representable in binary, so the product often lands just below the intended integer and the result is one byte short:
assert_eq!("2.01 KB".parse::().unwrap().0, 2009); // expected 2010
assert_eq!("2.05 GB".parse::().unwrap().0, 2_049_999_999); // expected 2_050_000_000
This isn't limited to contrived input. For two-decimal values between 0.01 and 99.99, 74 come out wrong in KB, 151 in MB, 224 in GB and 303 in TB. Binary units are unaffected for two decimal places because their factors are powers of two.
Changes
Replace the f64 multiplication with a small mul_decimal helper that does the computation in exact integer arithmetic. The whole part is multiplied with checked arithmetic. The fractional digits are folded right to left, flooring at each step, which gives the exact truncated value because floor((n + x) / 10) == floor((n + floor(x)) / 10) for integer n. Intermediate values stay below 10 * factor, which fits in a u64 for every unit up to EiB.
This replaces the integer-only special case added in #171, since the helper handles both cases. It also makes fractional inputs above 2^53 exact (e.g. "9007199254740993.5B").
Overflow still saturates at u64::MAX, as before.
Input validation is unchanged: number.parse::() still runs first, so malformed inputs such as "1.3.4" or "." are still rejected.
Tests
Unit tests for the inputs above, including leading/trailing ., long fractions, and saturation.
A quickcheck property comparing mul_decimal with an independent u128 computation across all units.
I checked that the new tests fail against the previous implementation.
Checks run: cargo test --workspace with --no-default-features and --all-features, doc tests, cargo clippy --workspace --all-targets (both feature sets), cargo fmt --check, cargo doc with -D warnings, tests on the 1.85 MSRV toolchain, and the thumbv6m-none-eabi no-std build.