Skip to content
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

Alternative fix for int64toString #367

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Conversation

tpcstld
Copy link
Contributor

@tpcstld tpcstld commented Aug 24, 2022

Summary

Borrowed from protocolbuffers/protobuf#8170, this PR is an alternative approach for #366. (I'm the colleague.)

The error in this optimization is that it incorrectly assumes that bitsLow is never negative, but that can occur when the highest bit is set due to 2s complement representation.

Test Plan

$ make
...
@protobuf-ts/runtime: 'test-node' ...
@protobuf-ts/runtime: Jasmine started
@protobuf-ts/runtime: Executed 622 of 622 specs SUCCESS in 0.45 sec.

Copy link
Owner

@timostamm timostamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the correct fix to me. Thanks!

@timostamm timostamm merged commit b54b84b into timostamm:master Aug 29, 2022
@tpcstld
Copy link
Contributor Author

tpcstld commented Sep 6, 2022

@timostamm Could we publish a new version of the package on NPM with this fix?

@timostamm
Copy link
Owner

Of course, it's released in v2.8.1.

@jcready
Copy link
Contributor

jcready commented Jul 14, 2023

This causes incorrect output on platforms w/o BigInt support for the following tests:

it('should toString() max unsigned int64', function () {
let uLong = new PbULong(-1, -1);
expect(uLong.toString()).toBe('18446744073709551615');
});

Expected '-1' to be '18446744073709551615'.

it('should toString() min signed int64', function () {
let uLong = new PbLong(0, -2147483648);
expect(uLong.toString()).toBe('-9223372036854775808');
});

Expected '--9223372036854776000' to be '-9223372036854775808'.


We can get the correct output if we modify the int64toString to always cast the high and low bits to be unsigned without having to change the if condition:

    bitsLow = bitsLow >>> 0;
    bitsHigh = bitsHigh >>> 0;
    if (bitsHigh <= 0x1FFFFF) {
        return '' + (TWO_PWR_32_DBL * bitsHigh + bitsLow);
    }

Alternatively we only cast bitsHigh to unsigned inside the if condition:

    if ((bitsHigh >>> 0) <= 0x1FFFFF) {
        return '' + (TWO_PWR_32_DBL * bitsHigh + (bitsLow >>> 0));
    }

All the tests pass (both with and without BI support) using either approach. I'm not sure if one solution is preferable to the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants