Skip to content

Improved Number.prototype.toString for radix other than 10 #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented Feb 28, 2024

  • fix the conversions for integers and exact fractions
  • approximate approach for other cases.
  • bypass floating point conversions for JS_TAG_INT values
  • avoid divisions for base 10 integer conversions
  • fixes Improve number->string conversion #242

- fix the conversions for integers and exact fractions
- approximate approach for other cases.
- bypass floating point conversions for JS_TAG_INT values
- avoid divisions for base 10 integer conversions
- fixes quickjs-ng#242
@chqrlie
Copy link
Collaborator Author

chqrlie commented Feb 28, 2024

I purposely did not remove the radix argument from js_dtoa and js_dtoa1 because we should change the implementation to avoid relying on the C library snprintf which has some issues on Windows.

@bnoordhuis
Copy link
Contributor

You will want to update v8.txt in the top-level directory but otherwise nice work.

- fix the conversions for integers and exact fractions
- approximate approach for other cases.
- bypass floating point conversions for JS_TAG_INT values
- avoid divisions for base 10 integer conversions
- fixes quickjs-ng#242
@chqrlie
Copy link
Collaborator Author

chqrlie commented Feb 28, 2024

I updated v8.txt. Why does v8.sh tee the output to the console? Just showing the diff would suffice and avoid verbose listings.

- fix the conversions for integers and exact fractions
- approximate approach for other cases.
- bypass floating point conversions for JS_TAG_INT values
- avoid divisions for base 10 integer conversions
- fixes quickjs-ng#242
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Why does v8.sh tee the output to the console?

It made it easier for me to debug things.

quickjs.c Outdated
Comment on lines 11295 to 11296
if (frac * radix >= radix / 2) {
char nine = '0' + radix - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, this is sort of incorrect when radix > 10 but works okay in practice? I tried to deduce if you could get an accidental match in e.g. base-18 or base-36 (because then nine is 'A' or 'S') but I'm not sure.

I suppose it could also lead to somewhat unexpected results on EBCDIC systems but 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is incorrect indeed for bases larger than 10. I shall post a fix. The code should be

    char nine = digits[radix - 1];

I am using an array for the digit characters, so EBCDIC should work fine :)

- fix the conversions for integers and exact fractions
- approximate approach for other cases.
- bypass floating point conversions for JS_TAG_INT values
- avoid divisions for base 10 integer conversions
- fixes quickjs-ng#242
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work.

@bnoordhuis bnoordhuis merged commit 7dd2868 into quickjs-ng:master Mar 1, 2024
@chqrlie
Copy link
Collaborator Author

chqrlie commented Mar 1, 2024

LGTM. Nice work.

Thank you. For completeness, this implementation is partial, it is not fully precise for the last fractional digit or two because of precision issues. I will post a multi-precision version when I get it done.

@chqrlie chqrlie deleted the improve-number-tostring branch March 1, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve number->string conversion
3 participants