-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 Half CompareTo on negatives #53141
Conversation
Tagging subscribers to this area: @tannergooding, @pgovind |
@@ -385,6 +385,11 @@ public int CompareTo(object? obj) | |||
/// <returns>A value less than zero if this is less than <paramref name="other"/>, zero if this is equal to <paramref name="other"/>, or a value greater than zero if this is greater than <paramref name="other"/>.</returns> | |||
public int CompareTo(Half other) | |||
{ | |||
if (this == other) |
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.
It's not clear why this fix is correct.
float
and double
are both implemented as <, >, ==, NaN
(how the code is prior to this PR), which leads me to believe that <
or >
are incorrectly handling an equals case.
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.
@tannergooding What do you think we should do on this? I saw your comment above, but I wasn't sure if you were hoping @ntovas could share more info or if you had other ideas.
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.
As is, I don't believe this is the correct fix. It needs someone (either the PR author, myself, or @pgovind) to do the further investigation as to where the actual bug is and where the CompareTo
logic (which is identical to what float
/double
do) is failing.
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.
@ntovas Do you think you'd be able to investigate this further?
I'm closing this PR as we aren't confident in the fix. @ntovas please don't hesitate to open a new PR if you're able to address the feedback and questions posted here. Thank you! |
fix #49983