-
Notifications
You must be signed in to change notification settings - Fork 79
Levrage BigInt to represent int64/uint64 #1030
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
Merged
Merged
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 22 changed files in this pull request and generated no comments.
Files not reviewed (15)
- package.json: Language not supported
- rosidl_gen/templates/message.dot: Language not supported
- test/test-service-with-async-callback.js: Evaluated as low risk
- test/test-interactive.js: Evaluated as low risk
- test/test-fixed-array.js: Evaluated as low risk
- test/client_setup.js: Evaluated as low risk
- test/test-bounded-sequences.js: Evaluated as low risk
- test/test-security-related.js: Evaluated as low risk
- test/test-message-translator-primitive.js: Evaluated as low risk
- rosidl_gen/packages.js: Evaluated as low risk
- rosidl_gen/message_translator.js: Evaluated as low risk
- test/test-parameter-service.js: Evaluated as low risk
- test/test-service-introspection.js: Evaluated as low risk
- test/test-parameters.js: Evaluated as low risk
- test/test-primitive-msg-type-check.js: Evaluated as low risk
Comments suppressed due to low confidence (1)
rosidl_parser/rosidl_parser.js:49
- The
contextparameter is only available in Node.js versions >= 21.0.0.0. Ensure that the version check correctly handles this and thatcontext.sourceis always available.
return JSON.parse(str, (key, value, context) => {
minggangw
added a commit
that referenced
this pull request
Feb 18, 2025
Due to [double precision floating point format](https://en.wikipedia.org/wiki/Double_precision_floating-point_format), JavaScript can only safely represent integers [between](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER) -(2^53 – 1) to 2^53 – 1, meanwhile, `Number.MAX_SAFE_INTEGER` is defined to represent the maximum safe integer in JavaScript, which is 9007199254740991. Per ROS2 message types, it has [int64](https://github.com/ros2/common_interfaces/blob/rolling/std_msgs/msg/Int64.msg)/[uint64](https://github.com/ros2/common_interfaces/blob/rolling/std_msgs/msg/UInt64.msg), which may be out of the range above. For current implementation, we leverage [ref](https://github.com/node-ffi-napi/ref-napi) to read/write values from `int64_t` and `uint64_t` data of C++, which can be type of: 1. `number`: when value in [`-Number.MAX_SAFE_INTEGER` `Number.MAX_SAFE_INTEGER`]. 2. `string`: otherwise. This brings the problem that the type of int64/uint64 be ununified in JavaScript. Along with [BigInt](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt) introduced, it becomes possible to make the type consistent. This patch implements to leverage `BigInt` to represent int64/uint64, including: 1. Update `message.dot` to use `BigInt` when hitting int64/uint64. 2. Update `rosidl_parser.js` to convert int64/uint64 to string in JSON object. 3. Update `message_translator.js` to add `bigint` as primitive. 4. Update `parameter.js` to use `BigInt` for integer and its `ts` definition `parameter.d.ts`. 5. Update tests targeting on messages that include int64/uint64. Fix: #836, #1025
minggangw
added a commit
that referenced
this pull request
May 23, 2025
minggangw
added a commit
that referenced
this pull request
May 23, 2025
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.
Due to double precision floating point format, JavaScript can only safely represent integers between -(2^53 – 1) to 2^53 – 1, meanwhile,
Number.MAX_SAFE_INTEGERis defined to represent the maximum safe integer in JavaScript, which is 9007199254740991. Per ROS2 message types, it has int64/uint64, which may be out of the range above.For current implementation, we leverage ref to read/write values from
int64_tanduint64_tdata of C++, which can be type of:number: when value in [-Number.MAX_SAFE_INTEGERNumber.MAX_SAFE_INTEGER].string: otherwise.This brings the problem that the type of int64/uint64 be ununified in JavaScript. Along with BigInt introduced, it becomes possible to make the type consistent.
This patch implements to leverage
BigIntto represent int64/uint64, including:message.dotto useBigIntwhen hitting int64/uint64.rosidl_parser.jsto convert int64/uint64 to string in JSON object.message_translator.jsto addbigintas primitive.parameter.jsto useBigIntfor integer and itstsdefinitionparameter.d.ts.Fix: #836, #1025