Skip to content

Port bellard/quickjs "dtoa library" commits#1142

Merged
saghul merged 6 commits intoquickjs-ng:masterfrom
past-due:bellard_dtoa_port
Sep 25, 2025
Merged

Port bellard/quickjs "dtoa library" commits#1142
saghul merged 6 commits intoquickjs-ng:masterfrom
past-due:bellard_dtoa_port

Conversation

@past-due
Copy link
Copy Markdown
Contributor

@past-due past-due commented Aug 25, 2025 •
edited
Loading

Port bellard/quickjs dtoa library commits

added new dtoa library to print and parse float64 numbers. It is necessary to fix corner cases (e.g. radix != 10) and to have correct behavior regardless of the libc implementation.

Port of:
bellard/quickjs@9936606
bellard/quickjs@dbbca3d

Includes alternative fix for bellard/quickjs@638ec8c from 0191aea

Remaining TODO items (would appreciate a look @saghul @bnoordhuis):

  • Amalgamation fails due to duplicate symbols (not sure how you'd prefer to fix this?)
  • Codegen update This was actually a side effect of the root cause of the subsequent issue
  • New test error 🤔 (see below - would appreciate a look at this one)
Result: 65/78606 errors, 3796 excluded, 5454 skipped, 2 new
test262/test/built-ins/TypedArrayConstructors/internals/Set/resized-out-of-bounds-to-in-bounds-index.js:18: unexpected error: Test262Error: Expected SameValue(«0», «100») to be true
test262/test/built-ins/TypedArrayConstructors/internals/Set/resized-out-of-bounds-to-in-bounds-index.js:18: strict mode: unexpected error: Test262Error: Expected SameValue(«0», «100») to be true

@past-due past-due mentioned this pull request Aug 25, 2025
Open
@bnoordhuis
Copy link
Copy Markdown
Contributor

bnoordhuis commented Aug 31, 2025

I have a fix for the first item in bnoordhuis/quickjs@2c073d7 that you can cherry-pick.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Sep 1, 2025

Can you run make gen? Looks like we're one bug away!

@past-due past-due force-pushed the bellard_dtoa_port branch 2 times, most recently from 57ad718 to 45997cf Compare September 18, 2025 19:27
@past-due
Copy link
Copy Markdown
Contributor Author

past-due commented Sep 18, 2025 •
edited
Loading

It turns out that I missed that the bellard/quickjs js_atof still used JS_NewFloat64 where quickjs-ng requires js_number() (since #319). Fixing that resolved the new test error (and also eliminated the codegen changes).

Something to keep a look out for in the future - any ports from bellard/quickjs that use JS_NewFloat64 should be double-checked.

@past-due past-due marked this pull request as ready for review September 18, 2025 19:52
saghul
saghul approved these changes Sep 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@past-due
Copy link
Copy Markdown
Contributor Author

past-due commented Sep 25, 2025 •
edited
Loading

@bnoordhuis Anything else you're looking for to merge? (CI tests check out, and some local testing checks out fine as well.)

EDIT: Rebased on latest master

past-due and others added 6 commits September 25, 2025 14:37
@past-due
Port bellard/quickjs dtoa library commits
135e752 
added new dtoa library to print and parse float64 numbers. It is necessary to fix corner cases (e.g. radix != 10) and to have correct behavior regardless of the libc implementation.

Port of:
bellard/quickjs@9936606
bellard/quickjs@dbbca3d

Includes alternative fix for bellard/quickjs@638ec8c from 0191aea

Update CMakeLists.txt and meson.build

Co-Authored-By: Fabrice Bellard <fabrice@bellard.org>
@past-due
Re-apply 29b4533
5ec5e3a
@past-due
Remove unnecessary includes in dtoa.c
75bffc8
@past-due
Add dtoa to amalgam.js
c234035
@bnoordhuis @past-due
squash! fix duplicate symbols in amalgamation
4773a58
@past-due
Fix porting of new js_atof
ec59c5d 
Re-apply quickjs-ng#319, use of js_number()
@saghul saghul merged commit 844bb02 into quickjs-ng:master Sep 25, 2025
127 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saghul saghul saghul approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@past-due @bnoordhuis @saghul