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 JS_ToBigUint64 #416

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Add JS_ToBigUint64 #416

merged 1 commit into from
Jun 4, 2024

Conversation

saghul
Copy link
Contributor

@saghul saghul commented May 27, 2024

Fixes: #376

@saghul saghul requested a review from chqrlie May 27, 2024 08:11
@saghul
Copy link
Contributor Author

saghul commented May 28, 2024

Ping @chqrlie, can you PTAL?

quickjs.c Outdated
Comment on lines 12012 to 12025
int JS_ToBigUint64(JSContext *ctx, uint64_t *pres, JSValue val)
{
bf_t a_s, *a;

a = JS_ToBigIntFree(ctx, &a_s, js_dup(val));
if (!a) {
*pres = 0;
return -1;
}
bf_get_uint64(pres, a);
JS_FreeBigInt(ctx, a, &a_s);
return 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the expected behavior of JS_ToBigUint64 for values larger or equal to 264 and for negative values? As a matter of fact, the behavior of JS_ToBigInt64 for values outside the range of int64_t is unclear to me too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'd say it'd defined as extracting the value in the val BigInt JS object, right? Whatever a JS BigInt objectg can represent, that's what what you'll get here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but what if the BigInt value is out of range? Should these functions return the value modulo 264 as specified in ECMA 7.1.7 ToUint32 for example or should they clamp the value and return UINT64_MAX for values beyond and 0 for negative values.
I much prefer the former (modulo) and it seems to be what the implementation of JS_ToBigInt64 does, but your implementation of JS_ToBigUint64 does the second, which is inconsistent.
The implementation is somewhat tricky because the current implementation of BigInt uses sign+magnitude and a mantissa/exponent representation, but this will change when Fabrice completes his reimplementation of BigInt using a more standard approach and 2's complement representation.
What is the current use case for JS_ToBigUint64 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Looks like libbf would need to be extended then, since now all it does is set it to UINT64_MAX. It's also currently unused by quickjs itself, so there is that.

Regarding motivation / use case, I have none, personally. It was requested in #376 and it looked like low hanging fruit, but maybe it isn't after all 😅

Copy link
Collaborator

@chqrlie chqrlie May 30, 2024

Choose a reason for hiding this comment

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

Looking again, there are 2 abstract operations defined by ECMA with the corresponding semantics:

  • 7.1.15 ToBigInt64
  • 7.1.16 ToBigUint64
    Both compute the 64-bit value modulo 264, which means they can be implemented using the same internal function and a simple cast on the pointer argument:
int JS_ToBigUint64(JSContext *ctx, uint64_t *pres, JSValue val)
{
    return JS_ToBigInt64Free(ctx, (int64_t *)pres, js_dup(val));
}

Or pedantically correct but slower (I prefer the simpler version here above):

int JS_ToBigUint64(JSContext *ctx, uint64_t *pres, JSValue val)
{
    int64_t val;
    int res = JS_ToBigInt64Free(ctx, &val, js_dup(val));
    *pres = val;
    return res;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! Let's go with the simple one then!

@saghul
Copy link
Contributor Author

saghul commented Jun 4, 2024

@chqrlie Updated!

@saghul saghul merged commit e5673a8 into master Jun 4, 2024
47 checks passed
@saghul saghul deleted the JS_ToBigUint64 branch June 4, 2024 17:03
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.

[Feature Request] Add JS_ToBigUint64() function
2 participants