From 7853d8aad43b68b87be97a39c5d032cb02735e55 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Fri, 31 Jul 2026 09:55:54 -0700 Subject: [PATCH] human review: Rational.zig --- engine/src/Rational.zig | 226 ++++++++++++++++++++++++++++++---------- engine/src/engine.zig | 5 +- 2 files changed, 174 insertions(+), 57 deletions(-) diff --git a/engine/src/Rational.zig b/engine/src/Rational.zig index c3eed96..a73446e 100644 --- a/engine/src/Rational.zig +++ b/engine/src/Rational.zig @@ -30,11 +30,27 @@ pub const Error = error{ InvalidNumber, /// The exponent of an integer power did not fit the supported range. ExponentTooLarge, + /// More digits than `parseDecimal` will hold. Distinct from `InvalidNumber` + /// because the text is a perfectly good number; it is this parser that declines. + TooManyDigits, /// Square root of a negative value. Raised by the numeric model rather than /// checked by each caller, so the domain rule lives in one place. NegativeRoot, }; +/// Mantissa digits `parseDecimal` will hold. Past this the value is rejected rather +/// than truncated: a silently shortened literal is a wrong answer. +const max_parse_digits = 512; + +/// Largest decimal exponent `parseDecimal` accepts. `1e100000` already needs a +/// 42KB numerator, so this is generous rather than restrictive. +const max_parse_exponent = 100_000; + +/// Digit separators, accepted anywhere in numeric text. +fn isSeparator(c: u8) bool { + return c == '_' or c == ',' or c == ' '; +} + /// Numerator. Carries the sign of the value. num: Managed, /// Denominator. Always strictly positive. @@ -160,76 +176,89 @@ pub fn parse(allocator: Allocator, text: []const u8) Error!Rational { /// Parse a decimal numeric literal exactly. /// /// Accepts an optional sign, digits with an optional fractional part, and an -/// optional decimal exponent: `42`, `-1.25`, `1e5`, `1.5e-3`. Underscores, -/// commas and spaces are ignored as digit separators, matching the -/// tokenizer's accepted forms. +/// optional decimal exponent: `42`, `-1.25`, `1e5`, `1.5e-3`. Underscores, commas +/// and spaces are ignored anywhere, which is looser than the tokenizer's grouping +/// rules (FR-1.8): this sees text the tokenizer has already accepted, plus the +/// hand-written factor strings in `units.zig`. /// -/// The result is exact: `0.1` becomes `1/10`, not the nearest binary float. +/// The result is exact: `0.1` becomes `1/10`, not the nearest binary float. Going +/// through `std.fmt.parseFloat` would defeat the purpose of this tier, since the +/// binary approximation is what the exact tier exists to avoid. +/// +/// One pass over the text. It used to make two copies, one to strip separators and +/// another to strip the decimal point, in two 512-byte buffers. pub fn parseDecimal(allocator: Allocator, text: []const u8) Error!Rational { - var buf: [512]u8 = undefined; - var len: usize = 0; - for (text) |c| { - if (c == '_' or c == ',' or c == ' ') continue; - if (len >= buf.len) return Error.InvalidNumber; - buf[len] = c; - len += 1; - } - var s = buf[0..len]; - if (s.len == 0) return Error.InvalidNumber; - - var negative = false; - if (s[0] == '+' or s[0] == '-') { - negative = s[0] == '-'; - s = s[1..]; - if (s.len == 0) return Error.InvalidNumber; - } - - // Split off the exponent. - var exponent: i64 = 0; - var mantissa = s; - if (std.mem.indexOfAny(u8, s, "eE")) |e_idx| { - mantissa = s[0..e_idx]; - const exp_text = s[e_idx + 1 ..]; - if (exp_text.len == 0) return Error.InvalidNumber; - exponent = std.fmt.parseInt(i64, exp_text, 10) catch return Error.InvalidNumber; - if (exponent > 100_000 or exponent < -100_000) return Error.ExponentTooLarge; - } - if (mantissa.len == 0) return Error.InvalidNumber; - - // Split the mantissa on the decimal point. - var digits_buf: [512]u8 = undefined; - var digits_len: usize = 0; + // The mantissa's digits, with separators and the point removed. + var digits: [max_parse_digits]u8 = undefined; + var digit_count: usize = 0; var frac_digits: usize = 0; var seen_dot = false; - for (mantissa) |c| { + var negative = false; + + var i: usize = 0; + while (i < text.len and isSeparator(text[i])) i += 1; + if (i < text.len and (text[i] == '+' or text[i] == '-')) { + negative = text[i] == '-'; + i += 1; + } + + // Mantissa, stopping at an exponent marker. + while (i < text.len) : (i += 1) { + const c = text[i]; + if (isSeparator(c)) continue; + if (c == 'e' or c == 'E') break; if (c == '.') { if (seen_dot) return Error.InvalidNumber; seen_dot = true; continue; } - if (c < '0' or c > '9') return Error.InvalidNumber; - if (digits_len >= digits_buf.len) return Error.InvalidNumber; - digits_buf[digits_len] = c; - digits_len += 1; + if (!std.ascii.isDigit(c)) return Error.InvalidNumber; + if (digit_count == digits.len) return Error.TooManyDigits; + digits[digit_count] = c; + digit_count += 1; if (seen_dot) frac_digits += 1; } - if (digits_len == 0) return Error.InvalidNumber; + if (digit_count == 0) return Error.InvalidNumber; + + // Exponent, accumulated directly rather than through a second buffer. The + // bound is checked as it grows, so the accumulator cannot overflow. + var exponent: i64 = 0; + if (i < text.len) { + i += 1; // the 'e' or 'E' + var exponent_negative = false; + while (i < text.len and isSeparator(text[i])) i += 1; + if (i < text.len and (text[i] == '+' or text[i] == '-')) { + exponent_negative = text[i] == '-'; + i += 1; + } + var exponent_digits: usize = 0; + while (i < text.len) : (i += 1) { + const c = text[i]; + if (isSeparator(c)) continue; + if (!std.ascii.isDigit(c)) return Error.InvalidNumber; + exponent = exponent * 10 + (c - '0'); + if (exponent > max_parse_exponent) return Error.ExponentTooLarge; + exponent_digits += 1; + } + if (exponent_digits == 0) return Error.InvalidNumber; + if (exponent_negative) exponent = -exponent; + } var num = try Managed.init(allocator); errdefer num.deinit(); - num.setString(10, digits_buf[0..digits_len]) catch return Error.InvalidNumber; + num.setString(10, digits[0..digit_count]) catch return Error.InvalidNumber; var den = try Managed.initSet(allocator, 1); errdefer den.deinit(); - // value = digits / 10^frac_digits * 10^exponent + // value = digits / 10^frac_digits * 10^exponent, so one scale factor does both. + // The exponent is bounded above and `frac_digits` cannot exceed the digit + // buffer, so the magnitude fits a u32 with four orders of magnitude to spare. const net_exp: i64 = exponent - @as(i64, @intCast(frac_digits)); if (net_exp != 0) { - const magnitude: u64 = @intCast(@abs(net_exp)); - if (magnitude > std.math.maxInt(u32)) return Error.ExponentTooLarge; var scale = try Managed.initSet(allocator, 10); defer scale.deinit(); - try scale.pow(&scale, @intCast(magnitude)); + try scale.pow(&scale, @intCast(@abs(net_exp))); if (net_exp > 0) { try num.mul(&num, &scale); } else { @@ -371,18 +400,16 @@ pub fn div(allocator: Allocator, a: Rational, b: Rational) Error!Rational { } pub fn negate(allocator: Allocator, a: Rational) Error!Rational { - var result = try a.clone(); + var result = try a.cloneWith(allocator); errdefer result.deinit(); result.num.negate(); - _ = allocator; return result; } pub fn abs(allocator: Allocator, a: Rational) Error!Rational { - var result = try a.clone(); + var result = try a.cloneWith(allocator); errdefer result.deinit(); result.num.abs(); - _ = allocator; return result; } @@ -706,10 +733,8 @@ pub fn toScientificString( // It can be off by one either way, which the loop below corrects. const num_bits: i64 = @intCast(magnitude.bitCountAbs()); const den_bits: i64 = @intCast(self.den.bitCountAbs()); - const log10_of_2 = 0.30102999566398120; - var exponent: i64 = @intFromFloat( - @floor(@as(f64, @floatFromInt(num_bits - den_bits)) * log10_of_2), - ); + const bit_difference: f64 = @floatFromInt(num_bits - den_bits); + var exponent: i64 = @intFromFloat(@floor(bit_difference * @log10(2.0))); const wanted: i64 = @intCast(significant_digits); // The exponent has to be settled on the TRUNCATED scaling, not the rounded @@ -982,6 +1007,72 @@ test "parseDecimal: rejects malformed input" { try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "abc")); try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "-")); try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "1e")); + try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "1e+")); + try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "1e2x")); + try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, ".")); + try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "+")); + // Separators alone are not a number. + try testing.expectError(Error.InvalidNumber, Rational.parseDecimal(alloc, "_,_")); +} + +test "parseDecimal: a valid number with too many digits says so" { + // Distinct from InvalidNumber: the text is a number, this parser just will not + // hold it. Reporting "invalid number" for 600 good digits was a wording bug. + var long: [max_parse_digits + 1]u8 = undefined; + @memset(&long, '9'); + try testing.expectError(Error.TooManyDigits, Rational.parseDecimal(alloc, &long)); + + // One digit fewer is fine, and exact. + var at_limit: [max_parse_digits]u8 = undefined; + @memset(&at_limit, '9'); + var value = try Rational.parseDecimal(alloc, &at_limit); + defer value.deinit(); + try testing.expect(value.isInteger()); + + // Separators do not count against the limit, since they are not digits. + var with_separators: [max_parse_digits * 2]u8 = undefined; + for (0..max_parse_digits) |k| { + with_separators[k * 2] = '9'; + with_separators[k * 2 + 1] = '_'; + } + var separated = try Rational.parseDecimal(alloc, &with_separators); + defer separated.deinit(); + try testing.expect(separated.isInteger()); +} + +test "parseDecimal: the exponent is bounded, and separators inside it are ignored" { + var ten_billion = try Rational.parseDecimal(alloc, "1e1_0"); + defer ten_billion.deinit(); + try expectFrac("10000000000", ten_billion); + + var spaced = try Rational.parseDecimal(alloc, "1e 5"); + defer spaced.deinit(); + try expectFrac("100000", spaced); + + // Leading zeros in the exponent do not overflow the accumulator. + var padded = try Rational.parseDecimal(alloc, "1e0000000000000000005"); + defer padded.deinit(); + try expectFrac("100000", padded); + + try testing.expectError( + Error.ExponentTooLarge, + Rational.parseDecimal(alloc, "1e100001"), + ); + try testing.expectError( + Error.ExponentTooLarge, + Rational.parseDecimal(alloc, "1e-100001"), + ); + // Absurd exponents are caught while accumulating, not by overflowing an i64. + try testing.expectError( + Error.ExponentTooLarge, + Rational.parseDecimal(alloc, "1e99999999999999999999999999"), + ); +} + +test "parseDecimal: a sign after separators still applies" { + var negative = try Rational.parseDecimal(alloc, " -1.5"); + defer negative.deinit(); + try expectFrac("-3/2", negative); } test "add: the classic binary float failure is exact here" { @@ -1105,6 +1196,29 @@ test "negate and abs" { try expectFrac("3/4", b); } +test "negate and abs allocate from the allocator they are given" { + // They used to call `clone()`, which copies into the operand's allocator, and + // discarded the parameter with `_ = allocator`. The leak detector is the + // assertion here: nothing deinits the results, so the arena has to own them. If + // they came from `alloc` instead, this fails as a leak. + var arena_state = std.heap.ArenaAllocator.init(alloc); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var value = try Rational.initRatio(alloc, 3, 4); + defer value.deinit(); + + const negated = try Rational.negate(arena, value); + try expectFrac("-3/4", negated); + + const magnitude = try Rational.abs(arena, negated); + try expectFrac("3/4", magnitude); + + // The operand is untouched: these return a fresh value rather than mutating one, + // which is what makes them safe to call on a stored variable or on `Ans`. + try expectFrac("3/4", value); +} + test "powInt: positive exponent" { var a = try Rational.initInt(alloc, 2); defer a.deinit(); diff --git a/engine/src/engine.zig b/engine/src/engine.zig index 88ee6b4..b7f68ac 100644 --- a/engine/src/engine.zig +++ b/engine/src/engine.zig @@ -86,11 +86,14 @@ pub fn phrase(err: Error) []const u8 { error.DomainError => "domain error", error.Overflow => "overflow", error.InvalidOperandType => "invalid operand type", - // These two used to be flattened into Overflow and DomainError by a mapping + // These used to be flattened into Overflow and DomainError by a mapping // between error sets. The specific wording is the whole reason the numeric // tier bothered to distinguish them. error.ExponentTooLarge => "the exponent is too large to compute", error.NegativeRoot => "square root of a negative number", + // A valid number with more digits than the exact parser will hold, which is + // not the same as a malformed one. + error.TooManyDigits => "too many digits", // Units error.UnknownUnit => "unknown unit",