-
Notifications
You must be signed in to change notification settings - Fork 79
Leverage BigInt for Time/Duration #1039
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
Conversation
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 12 out of 12 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (6)
lib/duration.js:40
- While the upper bound is checked, consider whether negative duration values should also be validated against a lower bound (if the underlying C time representation has one) or be documented as allowed.
if (total >= 2n ** 63n) {
lib/time.js:99
- Verify that using BigInt division in the secondsAndNanoseconds getter yields the correct integer seconds when dividing by S_TO_NS, particularly for very large nanosecond values.
const nanoseconds = this._nanoseconds;
lib/node.js:1474
- Converting the BigInt seconds value to a Number may lose precision for unexpectedly large time values; please confirm that this conversion is acceptable in this context.
sec: Number(secondsAndNanos.seconds),
lib/action/server.js:271
- Ensure that converting the BigInt seconds from secondsAndNanos to a Number does not introduce precision loss when interfacing with the underlying C time point, especially in edge cases.
sec: Number(secondsAndNanos.seconds),
test/test-time-source.js:75
- [nitpick] Consider including an additional test to verify the behavior when the difference exceeds the 1e9 nanoseconds threshold, ensuring that the comparison logic remains robust for larger time differences.
assert.ok(sysNow.nanoseconds - now.nanoseconds < 10n ** 9n);
lib/time_source.js:65
- The conditional is incorrectly checking the node type; replace it with 'if (!(node instanceof rclnodejs.ShadowNode))' to properly validate that the provided node is of the correct type.
if ((!node) instanceof rclnodejs.ShadowNode) {
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 12 out of 12 changed files in this pull request and generated no comments.
Per current implementation, the `Time`/`Duration` in rclnodejs module may be initialized with two parts, which are `seconds` and `nanoseconds` using type of `number` or `string`, this is because the range limitation of integer that JavaScript can [represents](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER). While, after [`BigInt`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt) introduced, we can change it to type of `bigint` only, which aligns with other ROS2 clients, like `rclpy`. This patch implements: 1. Leverage `BigInt` to initialize `Time`/`Duration` in nanoseconds from JavaScript side. 2. Change `Clock` and `TimeSource` accordingly. 3. Get the period from `v8::BigInt` object as `int64_t` and pass it to `rcl` from C++ side. 4. Update the related unit tests accordingly. 5. Remove dependency, `int64-napi`, which was used to pass `int64_t` from JavaScript to C++. Fix: #1040
Per current implementation, the
Time/Durationin rclnodejs module may be initialized with two parts, which aresecondsandnanosecondsusing type ofnumberorstring, this is because the range limitation of integer that JavaScript can represents. While, afterBigIntintroduced, we can change it to type ofbigintonly, which aligns with other ROS2 clients, likerclpy.This patch implements:
BigIntto initializeTime/Durationin nanoseconds from JavaScript side.ClockandTimeSourceaccordingly.v8::BigIntobject asint64_tand pass it torclfrom C++ side.int64-napi, which was used to passint64_tfrom JavaScript to C++.Fix: #1040