Very crude initial support for BigInt (if present)#252
Open
CyDragon80 wants to merge 3 commits intosidorares:masterfrom
Open
Very crude initial support for BigInt (if present)#252CyDragon80 wants to merge 3 commits intosidorares:masterfrom
CyDragon80 wants to merge 3 commits intosidorares:masterfrom
Conversation
added 2 commits
November 20, 2018 17:30
…efficient string conversions with Long.js code.
Author
|
On further thought, it would probably be more consistent and deterministic to have an option for 64 bit return types that is an enum of 'number', 'bigint', and 'longjs' (perhaps deprecate ReturnLongjs). The automagic flipping between number and bigint on certain conditions could potentially be confusing. Thoughts? |
Author
|
Not seeing Travis results, but I did test on Node 6 and 10. BigInt specific tests only run on 10+ as BigInt is not defined previous to that and are automatically skipped. |
CyDragon80
referenced
this pull request
in dbusjs/node-dbus-next
Feb 6, 2019
(Breaking change) Always use BigInt to represent dbus int64 and uint64 types. Remove Long.js dependency.
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
1 participant
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.
Inspired by #248
This an initial attempt to allow the use of BigInt (if it is available) without breaking backwards compatibility. At present it uses strings to convert between Long.js and BigInt. At some point that could probably be replaced with more efficient mathematic operations or some kind of BigInt functionality, but this is a start at least. (Perhaps in future, it will be primarily BigInt with Long.js as a fallback? Guess time will tell.)
Not sure if there should be an option to always unmarshall to BigInt, or if automatically flipping from number to BigInt when needed is fine.