BigInt-related API fixes and non-breaking changes#285
Open
mildsunrise wants to merge 3 commits intoromgrk:masterfrom
Open
BigInt-related API fixes and non-breaking changes#285mildsunrise wants to merge 3 commits intoromgrk:masterfrom
mildsunrise wants to merge 3 commits intoromgrk:masterfrom
Conversation
Owner
|
You can include the breaking changes here directly, I'll release 1.0 right after it's merged. |
Contributor
Author
|
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 |
Owner
|
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. |
Contributor
Author
|
Perfect, done. This seems to be working but I'll test it some more to make sure. |
romgrk
reviewed
May 4, 2021
Owner
romgrk
left a comment
romgrk
left a comment
There was a problem hiding this comment.
One comment but the rest seems good :)
| if (value->IsBigInt()) | ||
| return value.As<v8::BigInt>()->Int64Value(); | ||
| return Nan::To<int32_t>(value).ToChecked(); | ||
| } |
Owner
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?
Contributor
Author
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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