BigInt-related API fixes and non-breaking changes #285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You can include the breaking changes here directly, I'll release 1.0 right after it's merged. |
|
hmm I would wait a bit before the breaking release, just in case there are more breaking changes we haven't discovered yet? either way, +1 |
|
The breaking changes need to land on master anyway, even if we wait before the release. You can add them here directly, it will be simpler. |
|
Perfect, done. This seems to be working but I'll test it some more to make sure. |
romgrk
reviewed
May 4, 2021
| if (value->IsBigInt()) | ||
| return value.As<v8::BigInt>()->Int64Value(); | ||
| return Nan::To<int32_t>(value).ToChecked(); | ||
| } |
There was a problem hiding this comment.
This one feels a bit weird, with the name being ToInt32 but it returns an int64_t. Maybe it should return an int32_t?
side-effects: - g_value_info_get_value() returns an int64, but enums are still 32 bit, so we need to cast to Number
turns out ToBigInt() and ToNumber() doesn't perform conversion but only 'implicit' conversion which is not useful. create helpers to extract the numbers from V8 and use them everywhere (except in enum, flags, unichar)
mildsunrise
force-pushed
the
bigint-fixes
branch
from
355ca92
to
515d6a8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BigInt-related API fixes and (for now, only) non-breaking changes.
input conversions for BigInt:
all integer types (8, 16, 32, 64) now accept bigint and number
64-bit integer types ([u]long, [u]int64, gtype) now convert the value to BigInt and then call
[U]Int64Value()(the other integer types still convert to Number first, which means IIRC that if an out-of-range BigInt is passed, they'll get converted to trash rather than the low bits, as the user would expect. but it's better for performance and user can just
& 0xFFFFFFFFn)GType consistency (these are breaking, but very little used so I think a fix like this is justified):
GValueToV8now convertsGTypeto BigInt to be consistentGTypes only support conversion from BigInt nowI've also annotated the places that should be changed to completely support BigInt, but would be a breaking change.
other non-BigInt related fixes:
CanConvertV8ToGValue