From 638793142d1b9668df6f9f5f326832bab7d44cdd Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Fri, 28 Aug 2026 10:33:02 -0700 Subject: [PATCH] human review: engine and c_api --- .kiro/specs/calculator/design.md | 2 +- .kiro/specs/calculator/tasks.md | 27 ++++++++++++++++++++++++ engine/src/engine.zig | 35 ++++++++++++++++---------------- 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/.kiro/specs/calculator/design.md b/.kiro/specs/calculator/design.md index afa7f9d..9ca2603 100644 --- a/.kiro/specs/calculator/design.md +++ b/.kiro/specs/calculator/design.md @@ -75,7 +75,7 @@ build.zig (workspace root) | `units.zig` | Unit conversion tables and resolver | | `financial.zig` | CAGR, TVM, compound interest, amortization, and money display (cents are a property of money, not of a screen) | | `message.zig` | Gone: the wording lives in `engine.zig` beside the `Error` union it words | -| `engine.zig` | Public API surface (Zig-native): the module re-exports, the `Error` union, and `phrase` | +| `engine.zig` | Public API surface (Zig-native): the seven modules a frontend imports, nine name aliases, the derived `Error` union, and `phrase` | | `c_api.zig` | `extern "C"` wrappers for JNI/FFI consumers | Every module declares its own error set; there is no shared one (section 10). diff --git a/.kiro/specs/calculator/tasks.md b/.kiro/specs/calculator/tasks.md index 4404caa..8d2244b 100644 --- a/.kiro/specs/calculator/tasks.md +++ b/.kiro/specs/calculator/tasks.md @@ -914,8 +914,35 @@ plans a `--raw` flag, but today it is exercised only by tests. 100% line coverage, engine 99.44%. CLI output byte-identical across all five rows, both byte orders, ASCII packing and the multi-base standard-mode view. +### Task 5.24: The engine's surface is what someone imports + +`engine.zig` re-exported thirteen modules; six had no importer outside the engine. +`Rational`, `tokenizer`, `ast`, `bitwise` and `Parser` had none at all, and `number` +was reached only through the `Number` alias. A frontend evaluates with `evalString` +rather than building a tree, and computes on `Number` rather than on the rational tier +underneath it, so none of those were ever going to be asked for. + +What kept them compiling is `test { testing.refAllDecls(@This()); }`, which analyses +every declaration in the file. That is a useful idiom and it is also why nothing +flagged six dead exports: they were referenced, just not by anyone outside. + +`number` and `Parser` stay as private imports, the first for the `Number` alias and +the second for the test that walks `Parser.Error`. The rest are gone from the surface +and still reachable by path from inside the engine. Test count is unchanged at 929, +which confirms the re-exports were not what pulled those files' tests into the binary: +each is imported by a module that remains, so it is part of the compilation either +way. + +`c_api.zig` was reviewed in the same pass and left alone. Its three exports +(`tally_eval`, `tally_result_free`, `tally_version`) are a reasonable shape on the +surface, but it has no tests, appears in no coverage report because no test target +builds the shared library, and predates Task 5.17's removal of engine-side display +defaults. It will be rewritten against a real caller when the Android app lands; open +items 12 and 15 record the test gap and the `FormatOptions` question. + ### Task 5.23: One answer per loan, and a principal column in whole cents + Open item 7 named two amortization defects. Both are real, and neither is the mechanism the item blamed. diff --git a/engine/src/engine.zig b/engine/src/engine.zig index 88bd0f5..322756e 100644 --- a/engine/src/engine.zig +++ b/engine/src/engine.zig @@ -5,31 +5,32 @@ const std = @import("std"); -// Vocabulary, lowest first. +// The engine's surface, lowest first: a caller writes `engine.units.convert` or +// `engine.financial.solveTvm`. Every module here has an importer outside the engine. pub const grouping = @import("grouping.zig"); pub const Integer = @import("Integer.zig"); -// Exact numeric model (design.md 2.7). The evaluator computes in these. -pub const Rational = @import("Rational.zig"); -pub const number = @import("number.zig"); -// Language layer. -pub const tokenizer = @import("tokenizer.zig"); -pub const ast = @import("ast.zig"); -pub const Parser = @import("Parser.zig"); pub const evaluator = @import("evaluator.zig"); -// Fixed-width integer operations, shared by both modes. -pub const bitwise = @import("bitwise.zig"); pub const programmer = @import("programmer.zig"); -// Domains and display. pub const float_interp = @import("float_interp.zig"); pub const units = @import("units.zig"); pub const financial = @import("financial.zig"); -// The modules above are the engine's surface: a caller writes `engine.units.convert` -// or `engine.financial.solveTvm`. The aliases below exist only for the handful of -// names used often enough that the module prefix is noise. There used to be a -// curated re-export of nearly every public declaration, which drifted: two thirds -// of it had no callers, and `Value` was re-exported after the type it named had -// stopped being the engine's result type. +// Reached only from inside this file. `Rational`, `tokenizer`, `ast`, `bitwise` and +// `Parser` were public too and no frontend imported any of them: a caller evaluates +// with `evalString` rather than building a tree, and computes on `Number` rather than +// on the rational tier underneath it. `refAllDecls` in the tests below analyses every +// declaration, which is what kept them compiling and hid that nothing wanted them. +// +// They are still reachable by path (`@import("Rational.zig")`) for anything inside +// the engine, and their tests still run: every one of them is imported by a module +// above, so it is part of the compilation either way. +const number = @import("number.zig"); +const Parser = @import("Parser.zig"); + +// The aliases below exist only for the handful of names used often enough that the +// module prefix is noise. There used to be a curated re-export of nearly every public +// declaration, which drifted: two thirds of it had no callers, and `Value` was +// re-exported after the type it named had stopped being the engine's result type. pub const BitWidth = Integer.BitWidth; pub const Environment = evaluator.Environment; pub const evalString = evaluator.evalString;