-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
assert: Fix EqualValues to handle overflow/underflow #1531
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.
Minor revision from me, and excuse as I become more familiar with helping to maintain the project.
36ab024
to
2d27e0f
Compare
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.
Apparently complex isn't numeric, so this still behaves incorrectly:
EqualValues(t, complex128(1e+100+1e+100i), complex64(complex(math.Inf(0), math.Inf(0))))
I'm open to not fixing this case. Also I wonder why complex isn't numeric.
The underlying function ObjectsAreEqualValues did not handle overflow/underflow of values while converting one type to another for comparison. For example: EqualValues(t, int(270), int8(14)) would return true, even though the values are not equal. Because, when you convert int(270) to int8, it overflows and becomes 14 (270 % 256 = 14) This commit fixes that by making sure that the conversion always happens from the smaller type to the larger type, and then comparing the values. Additionally, this commit also seperates out the test cases of ObjectsAreEqualValues from TestObjectsAreEqual. Fixes stretchr#1462
2d27e0f
to
4c4d011
Compare
Summary
The underlying function
ObjectsAreEqualValues
did not handle overflow/underflow of values while converting one type to another for comparison. For example:would return
true
, even though the values are not equal. Because, when you convertint(270)
toint8
, it overflows and becomes 14(270 % 256 = 14)
Changes
This commit fixes that by making sure that the conversion always happens from the smaller type to the larger type, and then comparing the values. Additionally, this commit also separates out the test cases of
ObjectsAreEqualValues
fromTestObjectsAreEqual
.Related issues
Closes #1462