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

VB -> C#: (int)Math.Round(x) is used instead of Convert.ToInt32(x) #1082

Open
TymurGubayev opened this issue Mar 6, 2024 · 5 comments
Open
Labels
VB -> C# Specific to VB -> C# conversion

Comments

@TymurGubayev
Copy link
Contributor

VB.Net input code

Dim i As Integer
Dim d As Decimal
i = d

Erroneous output

int i;
var d = default(decimal);
i = (int)Math.Round(d);

This isn't broken (I don't think so), just very confusing to suddenly see rounding where there previously was none.

Expected output

int i;
var d = default(decimal);
i = Convert.ToInt32(d);

This has the additional benefit of looking the same as the VB.NET code decompiled to C# looks like:
SharpLab

Interestingly, explicit operator int(Decimal value) seems to do things slightly differently, so (int)d may not be the same as Convert.ToInt32(d).

Details

  • Product in use: VS extension
  • Version in use: 9.2.5.0
  • Did you see it working in a previous version, which? No
  • Any other relevant information to the issue, or your interest in contributing a fix.
@TymurGubayev TymurGubayev added the VB -> C# Specific to VB -> C# conversion label Mar 6, 2024
@Sympatron
Copy link

Although they boil down to equivalent code I would still prefer Convert.ToInt32 if that is what the VB compiler uses. Then it's guaranteed to always be exactly equivalent (for all runtime implementations) and also more clear IMO.

@tats-u
Copy link

tats-u commented Apr 10, 2024

I forgot to say that I just meant that its priority is lower than you think. Both results are fine for me. Math.Round makes it clear for readers that VB's CInt uses the bankers' rounding instead of the truncation. Both have pros and cons.

@TymurGubayev
Copy link
Contributor Author

Both are equivalent.

Convert.ToInt32: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Convert.cs,11fd4940860023d5

(int): https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Decimal.cs,950

Math.Round: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Math.cs,85130852558b2bc9

Convert.ToInt32(1.5M) == 2
&&
(int)1.5M == 1 &&  decimal.ToInt32(1.5M) == 1
&&
(int)Math.Round(d) == 2

I forgot to say that I just meant that its priority is lower than you think. Both results are fine for me. Math.Round makes it clear for readers that VB's CInt uses the bankers' rounding instead of the truncation. Both have pros and cons.

I would need to check every time what's the default value for Math.Round's MidpointRounding, and it would still surprise me every time.

@tats-u
Copy link

tats-u commented Apr 10, 2024

I would need to check every time what's the default value for Math.Round's MidpointRounding

It confuses those who mainly use C/C++, Java, and JS, as you think.
Some other languages (e.g. Python 3 and Kotlin) treat their round function as bankers' rounding like C# and VB.
I assumed those who mainly use C# but do not get used to VB.
I think you might know this, but the implementation of the overload with that argument is independent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VB -> C# Specific to VB -> C# conversion
Projects
None yet
Development

No branches or pull requests

3 participants