-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Disable nullable comparison optimization for decimals #73536
Disable nullable comparison optimization for decimals #73536
Conversation
Have we done similar change (the regression) for VB? |
{ | ||
System.Console.Write(M(x) ? 1 : 0); | ||
} | ||
static bool M(decimal? x) => x == 0.0m; |
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.
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.
Consider testing x == 0.00m
and 0.00m == x
since the reported issue uses 0.00m
.
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.
Let's also test x != <constant>
and <constant> != x
.
C.Run(0.0m); | ||
C.Run(1m); | ||
C.Run(2m); | ||
C.Run(null); |
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.
or ConstantValueTypeDiscriminator.NUInt | ||
or ConstantValueTypeDiscriminator.SByte | ||
or ConstantValueTypeDiscriminator.Single | ||
or ConstantValueTypeDiscriminator.String |
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.
return constantValue is | ||
{ | ||
IsDefaultValue: false, | ||
Discriminator: ConstantValueTypeDiscriminator.Boolean |
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.
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.
Thanks. Looks like integers smaller than 32 bits are not optimized because they use Int32's equality operator, so I will remove them from the list.
No, the optimization hasn't been implemented for VB. |
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.
No additional comments beyond Chuck's
{ | ||
System.Console.Write(M(x) ? 1 : 0); | ||
} | ||
static bool M(short? x) => x == 1; |
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.
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.
That results in longer IL, you can see that in SharpLab. Same for other less-than-32bit ints. Doesn't seem to have any effect on the optimization as it's currently (or was) implemented. The cast creates a more complicated bound tree
{ | ||
System.Console.Write(M(x) ? 1 : 0); | ||
} | ||
static bool M(ushort? x) => x == 1; |
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.
{ | ||
System.Console.Write(M(x) ? 1 : 0); | ||
} | ||
static bool M(sbyte? x) => x == 1; |
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.
Please cherry pick to release/dev17.10 after merge |
Fixes #73510, a regression introduced by #68069.