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#: Error converting a For loop #602

Closed
FeaXR opened this issue Aug 3, 2020 · 6 comments
Closed

VB -> C#: Error converting a For loop #602

FeaXR opened this issue Aug 3, 2020 · 6 comments
Labels
compilation error A bug where the converted output won't compile VB -> C# Specific to VB -> C# conversion

Comments

@FeaXR
Copy link

FeaXR commented Aug 3, 2020

Input code

For i As Integer = 0 To i < OldWords.Length
            HTMLCode = HTMLCode.Replace(OldWords(i), NewWords(i))
        Next i

Erroneous output

 for (var i = 0, loopTo = i < OldWords.Length; i <= Conversions.ToInteger(loopTo); i++)
                HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);

Expected output

 for (var i = 0; i < OldWords.Length; i++)
            {
                HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);
            }

Details

  • Product in use: VS extension, VS Community 16.6.5
  • Version in use: 8.1.6.0
@FeaXR FeaXR added the VB -> C# Specific to VB -> C# conversion label Aug 3, 2020
@GrahamTheCoder
Copy link
Member

Hi thanks for getting in touch. Can you tell me about what problem the erroneous output is causing you? Is it that it doesn't compile (e.g. Missing reference to Conversions), does the wrong thing, or you just don't like the look of it?

I believe the loopTo variable more accurately reflects what VB does. I.e. If a method in the loop, or another thread changes the length of Old Words, I think your suggested output would behave differently to the VB.

The Conversions method looks entirely redundant, and shouldn't be generated if there's type information. Were the variables or properties defined with types?

@FeaXR
Copy link
Author

FeaXR commented Aug 4, 2020

The loopTo variable is non existent and never gets changed in the loop, thus rendering it useless. The OldWords array never changes, especially in the for loop.
Even if it did change, I believe

for (var i = 0; i < OldWords.Length; i++) { HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]); }
would still do the same thing, since it is checking if the iterator is greater than any possible index of the array.

@GrahamTheCoder GrahamTheCoder added the compilation error A bug where the converted output won't compile label Aug 4, 2020
@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Aug 4, 2020

Ah, now I'm reading on a proper-sized screen I see you've used a boolean as the limit on your for loop which is very unusual and will have surprising results (I think never entering the loop). Based on your expected output (converting it back to VB) your input should probably have been this:

For i As Integer = 0 To OldWords.Length - 1
    HTMLCode = HTMLCode.Replace(OldWords(i), NewWords(i))
Next

For that input, it does then convert to C# correctly - actual converted output:

for (int i = 0, loopTo = OldWords.Length - 1; i <= loopTo; i++)
    HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);

But even though I think it was a bug that lead here the bug report is still relevant. The VB you gave does compile (when the variables are defined), and is a feature people sometimes intentionally use in VB, so I'll try and improve the converter support around it at some point.

Hope that makes sense. Let me know if I've misunderstood, thanks for reporting!

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Aug 4, 2020

I've handled the boolean To expression case. The fix will be in the next release.
I'm not planning to handle the case where OldWords is not declared, since I only usually support very common non-compiling input and this is an edge case.

@FeaXR
Copy link
Author

FeaXR commented Aug 5, 2020

Yes, you understood correctly :)
Sadly I didn't really have control over the input, as I was just given the code and I just had to transfer it to C#. Thanks a lot for your quick reply and fix, what you are doing is amazing.

@Saibamen
Copy link
Contributor

@FeaXR: Please retest. New version is already available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compilation error A bug where the converted output won't compile VB -> C# Specific to VB -> C# conversion
Projects
None yet
Development

No branches or pull requests

3 participants