Fix Number toString scientific notation threshold and format#415
Merged
Conversation
- Use ES-style decimal/scientific formatting for numeric literals and values - Add coverage for large integers, small magnitudes, and string coercion
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces ad-hoc FloatToStr formatting with a new ES2026-aligned Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/units/Goccia.Values.Primitives.pas (1)
161-162: Use the exact spec pseudo-signature in the annotation.These comments are close, but the repo convention is the exact
SpecMethodName(specParams)form immediately above the implementation. Using the canonical signature here will keep spec-audit grepability consistent.As per coding guidelines, annotate each function implementing ECMAScript-specified behavior with
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above the function body, using the spec's pseudo-code method name and parameters exactly as written.Also applies to: 184-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.Primitives.pas` around lines 161 - 162, The comment above DoubleToESString uses a descriptive note instead of the required canonical spec pseudo-signature; replace it with an exact spec annotation in the repo convention format (for example: // ES2026 §6.1.6.1.13 Number::toString(x) ) placed immediately above the function body, and do the same for the other ECMAScript-implementing routines in the same area (the functions around the later numeric/string helper implementations referenced in your review) so each has the exact SpecMethodName(specParams) and section number as the single-line comment immediately above its implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.Primitives.pas`:
- Around line 194-317: Replace the JSON path that formats finite doubles with
your new ES formatter: find the code that calls FloatToStr(...) for serializing
numbers (the JSON stringify/serialize routine that handles finite numbers,
referenced around the FloatToStr use in Goccia.JSON) and call
DoubleToESString(AValue) instead; also add the unit that exposes
DoubleToESString to the uses clause if it isn't already referenced. Do the same
replacement at the other occurrence noted (the second FloatToStr site) so all
finite-number JSON serialization uses DoubleToESString and preserves existing
sign/NaN/Infinity handling.
---
Nitpick comments:
In `@source/units/Goccia.Values.Primitives.pas`:
- Around line 161-162: The comment above DoubleToESString uses a descriptive
note instead of the required canonical spec pseudo-signature; replace it with an
exact spec annotation in the repo convention format (for example: // ES2026
§6.1.6.1.13 Number::toString(x) ) placed immediately above the function body,
and do the same for the other ECMAScript-implementing routines in the same area
(the functions around the later numeric/string helper implementations referenced
in your review) so each has the exact SpecMethodName(specParams) and section
number as the single-line comment immediately above its implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e32f34e9-9d34-4482-91c3-8f2a9488cf8b
📒 Files selected for processing (3)
source/units/Goccia.Parser.passource/units/Goccia.Values.Primitives.pastests/built-ins/Number/prototype/toString.js
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 30 improved · 🔴 122 regressed · 234 unchanged · avg -1.8% arraybuffer.js — Interp: 🔴 4, 10 unch. · avg -3.1% · Bytecode: 🟢 12, 2 unch. · avg +13.7%
arrays.js — Interp: 🟢 1, 🔴 4, 14 unch. · avg -1.0% · Bytecode: 🟢 18, 1 unch. · avg +11.5%
async-await.js — Interp: 🔴 1, 5 unch. · avg -4.3% · Bytecode: 🟢 5, 1 unch. · avg +11.4%
base64.js — Interp: 🔴 4, 6 unch. · avg -2.8% · Bytecode: 🟢 10 · avg +12.1%
classes.js — Interp: 🔴 8, 23 unch. · avg -1.9% · Bytecode: 🟢 20, 11 unch. · avg +6.2%
closures.js — Interp: 🔴 6, 5 unch. · avg -3.4% · Bytecode: 🟢 10, 1 unch. · avg +11.3%
collections.js — Interp: 🟢 1, 🔴 2, 9 unch. · avg -1.3% · Bytecode: 🟢 11, 1 unch. · avg +11.4%
csv.js — Interp: 🟢 2, 🔴 2, 9 unch. · avg -0.3% · Bytecode: 🟢 13 · avg +13.1%
destructuring.js — Interp: 🟢 1, 🔴 9, 12 unch. · avg -1.6% · Bytecode: 🟢 22 · avg +8.3%
fibonacci.js — Interp: 🔴 5, 3 unch. · avg -3.3% · Bytecode: 🟢 8 · avg +11.7%
float16array.js — Interp: 🟢 2, 🔴 15, 15 unch. · avg -3.0% · Bytecode: 🟢 23, 🔴 4, 5 unch. · avg +6.2%
for-of.js — Interp: 🔴 1, 6 unch. · avg +0.4% · Bytecode: 🟢 7 · avg +12.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 1, 🔴 13, 28 unch. · avg -2.2% · Bytecode: 🟢 33, 9 unch. · avg +6.4%
json.js — Interp: 🔴 6, 14 unch. · avg -2.5% · Bytecode: 🟢 16, 4 unch. · avg +7.2%
jsx.jsx — Interp: 🔴 10, 11 unch. · avg -2.4% · Bytecode: 🟢 19, 2 unch. · avg +11.6%
modules.js — Interp: 🔴 2, 7 unch. · avg -1.6% · Bytecode: 🟢 9 · avg +14.0%
numbers.js — Interp: 🔴 7, 4 unch. · avg -6.5% · Bytecode: 🟢 10, 🔴 1 · avg +6.4%
objects.js — Interp: 🟢 2, 5 unch. · avg +2.0% · Bytecode: 🟢 5, 2 unch. · avg +6.0%
promises.js — Interp: 12 unch. · avg +0.0% · Bytecode: 🟢 11, 1 unch. · avg +7.2%
regexp.js — Interp: 🟢 4, 7 unch. · avg +1.8% · Bytecode: 🟢 11 · avg +14.0%
strings.js — Interp: 🟢 6, 🔴 10, 3 unch. · avg +12.3% · Bytecode: 🟢 18, 1 unch. · avg +7.9%
tsv.js — Interp: 🟢 1, 8 unch. · avg +1.7% · Bytecode: 🟢 9 · avg +13.9%
typed-arrays.js — Interp: 🟢 8, 🔴 5, 9 unch. · avg -0.0% · Bytecode: 🟢 8, 🔴 5, 9 unch. · avg +0.6%
uint8array-encoding.js — Interp: 🟢 1, 🔴 8, 9 unch. · avg -16.7% · Bytecode: 🟢 15, 🔴 1, 2 unch. · avg +23.6%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
SerializeJSONNumber now delegates to DoubleToESString (ES2026 §25.5.2.5 step 8.a calls ToString), removing the separate FloatToStr + NormalizeExponentNumber pipeline and its dead helpers. Spec annotation comments updated to the canonical format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.Primitives.pas`:
- Around line 242-305: The DoubleToESString routine uses DefaultFormatSettings
(FS) but doesn't enforce an invariant decimal separator, so locale settings can
break parsing; fix by setting FS.DecimalSeparator := '.' immediately after FS :=
DefaultFormatSettings in DoubleToESString so that subsequent calls to Str(...,
SciStr), TryStrToFloat(..., FS) and FloatToStr(..., FS) always use '.' as the
decimal separator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fac8b107-222c-4055-bd6c-d7e2d2b2e07d
📒 Files selected for processing (2)
source/units/Goccia.JSON.passource/units/Goccia.Values.Primitives.pas
FPC 3.2.2 aliases DefaultFormatSettings and FormatSettings via Absolute, so a locale with ',' as decimal separator would break the Str/TryStrToFloat round-trip. Pin FS.DecimalSeparator := '.' to match every other format-settings site in the codebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced May 6, 2026
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
1 participant
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.
Summary
Fixes #400.
DoubleToESStringper ES2026 §6.1.6.1.13 — integers in[1e15, 1e21)now use fixed-point notation instead of premature scientific notation, and scientific notation uses lowercasee+/e-per specFloatToStrinToStringLiteraland the parser's numeric property key paths, fixingString(n), template literals,JSON.stringify, and computed member names for large integersNumber.MAX_SAFE_INTEGER, negative large integers,5e-324, small-number thresholds, and implicit coercion paths