Conversation
|
Oh, there appearst to be a bug in the DSx driver that I just caught with this change: When we go to |
There was a problem hiding this comment.
Pull request overview
This PR modernizes internal VInt encoding/decoding and Duration’s internal nanosecond arithmetic by moving from Long to native bigint, while keeping the Duration public API (e.g., duration.nanoseconds returning Long) intact. It also replaces several util.format() calls in duration parsing/validation with template literals.
Changes:
- Refactor
utils.VIntCodingread/write paths to operate onbigint(including zig-zag encode/decode). - Update
Durationserialization/deserialization and builder math to usebigintinternally for nanoseconds. - Replace
util.format()usage in duration parsing/validation errors with template literals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/utils.js | Refactors VIntCoding implementation to use bigint instead of Long. |
| lib/types/duration.js | Switches internal nanosecond math + VInt interactions to bigint; updates error formatting strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update Vint encoding to use BigInt instead of Long.
Replace internal uses of Long to BigInt. This does not change the API
of the class. Replace util.format() to "`${}`" syntax.
In the DSx code, when addNanos received ABS(MIN_INT64) [ex. when parsing MIN_INT64 ns duration], it pared it into 64 bit type, which quietly converted it into MIN_INT64, rather than the expected ABS(MIN_INT64). This then allowed it to pass the check in `validateNanos`, as we were comparing negative value instead of the expected positive value. With the replacement of Long in favour of BigInt, we now correctly parse ABS(MIN_INT64) when creating a BigInt. This leads to incorrectly trigger the validate64 error check when parsing MIN_INT64 ns duration. This commit adds a check that take into account the difference between ABS(MIN_INT64) and MAX_INT64, by accepting ABS(MIN_INT64) only for negative durations.
Refactor the VintEncoding and duration to use BigInt instead of Long for the internal uses. No changes to the duration public API were made.
For duration I also refactor util.format into
`${}`syntaxFixes: #378
Refs: #41