Simplify number parsing #386
Conversation
chqrlie
force-pushed
the
simplify-js-atof
branch
from
b8d6018 to
6c25f26
Compare
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Only a partial review. I ran out of time.
quickjs.c
Outdated
| #define ATOD_TYPE_MASK (1 << 8) | ||
| #define ATOD_TYPE_FLOAT64 (0 << 8) | ||
| #define ATOD_TYPE_BIG_INT (1 << 8) |
There was a problem hiding this comment.
Since you're here... I never really liked this ATOD_TYPE_MASK thing because it's not really a mask, just a single bit discriminator to distinguish between doubles and bigints (and it's only used in one place.)
Related suggestion: replace int atod_type with BOOL want_bigint
There was a problem hiding this comment.
I changed the API and now use a flag ATOD_WANT_BIG_INT
| /* Return an exception in case of memory error. | ||
| Return JS_NAN if invalid syntax */ | ||
| // TODO(chqrlie) need more precise error message | ||
| static JSValue js_atof(JSContext *ctx, const char *p, const char *end, |
There was a problem hiding this comment.
js_atof mutates radix in a number of places. Shouldn't it do that only when radix != 0?
There was a problem hiding this comment.
radix was modified in different places for specific reasons, but only if 0 was passed as the argument value.
I changed the API to always pass the default radix in the range 2 to 36 and pass flags for optional prefix parsing for 0x, 0b and 0o. I also simplified legacy octal support in the parser only.
| else | ||
| return js_int32(0); | ||
| } | ||
| if (*p == '+' || *p == '-') { |
There was a problem hiding this comment.
Out of bounds reads from here on when p == end? Maybe it's better to get rid of end altogether and mandate that p points to a zero-terminated buffer? (Zero or some other sentinel.)
Only one call site (next_token) seems to pass a non-null end.
There was a problem hiding this comment.
I Agree the API is clunky. I shall try and simplify this.
There was a problem hiding this comment.
Actually, after careful review, passing the end of the array is required to distinguish embedded null bytes from the final null byte. I am adding an explanatory comment:
- `p` points to a null terminated UTF-8 encoded char array
- `end` points to the end of the array.
There is a null byte at `*end`, but there might be embedded null bytes
between `p` and `end` which must cause an exception if the
`ATOD_NO_TRAILING_CHARS` flag is not present.
9993d40 to
63ec7ee
Compare
- use single test in `js_strtod` loop. - use more explicit `ATOD_xxx` flags - remove `ATOD_TYPE_MASK`, use `ATOD_WANT_BIG_INT` instead - remove unused arguments `flags` and `pexponent` in `js_string_to_bigint` - merge `js_atof` and `js_atof2`, remove `slimb_t *pexponent` argument - simplify and document `js_atof` parser, remove cumbersome labels, - simplify `js_parseInt` test for zero radix for `ATOD_ACCEPT_HEX_PREFIX` - simplify `next_token` number parsing, handle legacy octal in parser only - simplify `JS_StringToBigInt`, use flags only. - remove unused `slimb_t exponent` token field - add number syntax tests
3 participants
js_strtodloop.ATOD_xxxflagsATOD_TYPE_MASK, useATOD_WANT_BIG_INTinsteadflagsandpexponentinjs_string_to_bigintjs_atofandjs_atof2, removeslimb_t *pexponentargumentjs_atofparser, remove cumbersome labels,js_parseInttest for zero radix forATOD_ACCEPT_HEX_PREFIXnext_tokennumber parsing, handle legacy octal in parser onlyJS_StringToBigInt, use flags only.slimb_t exponenttoken field