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

fix(NODE-3640): Fix Int32 constructor to coerce its argument to int32 #466

Merged
merged 5 commits into from
Oct 25, 2021

Conversation

gjchong25
Copy link
Contributor

Int32 constructor truncates its argument immediately to int32 so that out of bound values are dealt with properly. Tests for the max/min bounds as well as out of bound positive/negative values are added, existing tests that did not take into account this new functionality were updated.

@gjchong25 gjchong25 requested a review from dariakp October 22, 2021 17:27
@gjchong25 gjchong25 changed the title (NODE-3640) Fix Int32 constructor to coerce its argument to int32 fix(NODE-3640): Fix Int32 constructor to coerce its argument to int32 Oct 22, 2021
test/node/int_32_tests.js Outdated Show resolved Hide resolved
test/node/int_32_tests.js Outdated Show resolved Hide resolved
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 22, 2021
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Just a couple small suggestion and need to remove the stray .only; otherwise great job, thank you for improving the wording on the other test cases, too!

test/node/int_32_tests.js Outdated Show resolved Hide resolved
test/node/int_32_tests.js Outdated Show resolved Hide resolved
test/node/int_32_tests.js Outdated Show resolved Hide resolved
Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good with @dariakp's comments addressed :)

@gjchong25 gjchong25 requested a review from dariakp October 25, 2021 15:29
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@dariakp dariakp merged commit d388f1e into master Oct 25, 2021
@dariakp dariakp deleted the NODE-3640 branch October 25, 2021 19:59
@dariakp dariakp removed the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 25, 2021
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