Skip to content
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

Add custom printf version (Draft) #390

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented Apr 20, 2024

Add custom printf version

  • add js_snprintf, js_printf... to handle extra conversions:
    • support for wxx length modifier
    • support for %b and %B
    • %oa and %#oa to convert JSAtom values
    • %ps to convert JSString values
  • add dbuf_vprintf_fun replaceable dbuf_printf handler
  • change JS_DumpString to JS_FormatString to convert JSSAtom to quoted strings
  • change JS_AtomGetStrRT to JS_FormatAtom to convert JSAtom to strings
  • change JS_AtomGetStr to return direct string pointer for builtin atoms
  • remove print_atom
  • use custom conversions for trace messages and error messages
  • add support for %b, %B and w length modifier in std.printf
  • remove error handlers: JS_ThrowTypeErrorAtom and JS_ThrowSyntaxErrorAtom
  • add is_lower_ascii() and to_lower_ascii()
  • add floating point conversions and wide string conversions
  • unbreak compilation: prevent name collision on pow10
  • minimize vsnprintf calls in dbuf_vprintf_default

@chqrlie chqrlie force-pushed the custom-error-formating branch 3 times, most recently from 678d0a8 to ead95a7 Compare April 20, 2024 16:34
@bnoordhuis
Copy link
Contributor

I'm sorta lukewarm on this. Prior experiences with homegrown (s)printf implementations have been a mixed bag.

I mean, I get the appeal but complex features need to pay for themselves and I'm not quite seeing that. Is there one killer feature in particular that made you start on this?

@chqrlie
Copy link
Collaborator Author

chqrlie commented May 1, 2024

I'm sorta lukewarm on this. Prior experiences with homegrown (s)printf implementations have been a mixed bag.

I mean, I get the appeal but complex features need to pay for themselves and I'm not quite seeing that. Is there one killer feature in particular that made you start on this?

Hi @bnoordhuis

I understand your concerns, I have invested a hefty amount of work into this, both for pleasure (I like to hack printf for breakfast*) and for actual goals in this project:

  • fix portability issues with some printf implementations that have non standard or implementation defined behavior, not to mention obsolete implementations that lack C99 support.
  • fix the floating point to string conversions rounding issues. This occurs on Windows, but also in macOS: a current example is v8.sh failures on my laptop (addressed in Improve number to string conversions #400):
     === number-tostring-func.js
    +Failure (0.5.toFixed(0)): expected <"1"> found <"0">
    +Failure ((-0.5).toFixed(0)): expected <"-1"> found <"-0">
     === number-tostring-small.js
     === number-tostring.js
    +Failure (0.5.toFixed(0)): expected <"1"> found <"0">
    +Failure ((-0.5).toFixed(0)): expected <"-1"> found <"-0">
     === numops-fuzz-part1.js
    
  • more generally improve the number to string conversion that currently calls snprintf multiple times with varying precision settings to implement the minimum representation and the round away from zero semantics that are not supported by snprintf portably and are difficult to emulate.
  • to implement proper javascript rounding on RNDN architectures, we change the current rounding mode using fesetround(), which may not be thread-safe for pre-C23 targets.
  • avoid locale issues or handle them explicitly: the library printf uses the current locale for floating point conversions, which may produce incorrect output if the decimal point differs from '.'. Changing the locale is not thread safe and passing the locale dynamically is not portable. If the host application uses locales for its own purposes, QuickJS would be affected beyond fixing (actually I hope I fixed this in Improve number to string conversions #400).
  • radix conversions are also imperfect and produce subtly different output from node. My implementation of floating point conversions will help with that too.
  • conversions from string to numbers (js_strtod) should also be rewritten to ensure proper rounding in a portable fashion to all targets and remove locale dependencies. This would also avoid the copying pass to remove the _ separators.
  • extensions for JSAtoms, JSStrings and possibly other objects allow for much simpler code for traces and error message construction.

The implementation in the current PR still uses the library snprintf for floating point values, but I am testing a complete version and will post that shortly. I shall also add a complete test suite with 450000 tests: qjs_printf passes and is on average 50% faster than the C library versions on Apple and linux).

As you have noticed, Fabrice and I tried hard to avoid depending on other packages to keep the project self contained and reliable. printf is a remaining stone is our shoes that I would like to get rid of too.


(*) Implementing printf for breakfast is a good challenge for C experts, getting all the integer conversions right is feasible, but handling the floating point conversions correctly (and efficiently) does require time and effort :)

@bnoordhuis
Copy link
Contributor

@chqrlie I don't have much time this week and (maybe) next week (school holidays) but I'll try to review this at the first possible opportunity.

@chqrlie
Copy link
Collaborator Author

chqrlie commented May 7, 2024

@chqrlie I don't have much time this week and (maybe) next week (school holidays) but I'll try to review this at the first possible opportunity.

Thank you. I have not plugged js_dtoa into this yet, so you may want to wait for a final version... I'll keep you posted.

@chqrlie chqrlie changed the title Add custom printf version Add custom printf version (Draft) May 8, 2024
- add `js_snprintf`, `js_printf`... to handle extra conversions:
  - support for wxx length modifier
  - support for `%b` and `%B`
  - `%oa` and `%#oa` to convert `JSAtom` values
  - `%ps` to convert `JSString` values
- add `dbuf_vprintf_fun` replaceable `dbuf_printf` handler
- change `JS_DumpString` to `JS_FormatString` to convert `JSSAtom` to quoted strings
- change `JS_AtomGetStrRT` to `JS_FormatAtom` to convert `JSAtom` to strings
- change `JS_AtomGetStr` to return direct string pointer for builtin atoms
- remove `print_atom`
- use custom conversions for trace messages and error messages
- add support for `%b`, `%B` and `w` length modifier in `std.printf`
- remove error handlers: `JS_ThrowTypeErrorAtom` and `JS_ThrowSyntaxErrorAtom`
- add `is_lower_ascii()` and `to_lower_ascii()`
- add floating point conversions and wide string conversions
- unbreak compilation: prevent name collision on pow10
- minimize `vsnprintf` calls in `dbuf_vprintf_default`
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.

2 participants