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

Implement correct string comparisons for VB #309

Merged
merged 2 commits into from May 7, 2019

Conversation

Projects
None yet
2 participants
@mrmonday
Copy link
Contributor

commented May 3, 2019

Closes #308

Problem

String comparison behaviour for converted VB -> C# code does not match VB's behaviour.

Solution

Added a special case for string equality. Added a special case to that to produce string.IsNullOrEmpty() when we can to produce slightly nicer code.

  • At least one test covering the code changed
  • All tests pass
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Weird, the tests passed locally... Guess it didn't actually run them all. I'll fix them on Monday, unless you get to it before me.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Also see https://github.com/icsharpcode/CodeConverter/compare/string-comparison?expand=1

In that branch I was trying to handle a bunch of cases without calling VB's method. Since string comparison is such a common thing I thought it'd be good to try. But I didn't quite get around to finishing it.

@mrmonday mrmonday force-pushed the mrmonday:308-string-comparisons branch from b1242cf to 7462e7c May 6, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I've rebased and pushed fixed the test case.

I think your branch handles more cases of implicit conversions, which this doesn't.

Unfortunately I don't think we can do much more than this PR does, in terms of not calling the method - in general it's impossible to know if a variable will be null at runtime (at least until we have non-nullable types). Without that knowledge, we have to call the method (or effectively duplicate the logic). I can think of two additional cases where we could handle this differently:

  • Comparison against any string or character literal could become ==/!=
  • Comparison against "known null or empty" values, beyond just literals (though this produces potentially unintuitive output)

I'll add some code to handle the first case.

@GrahamTheCoder GrahamTheCoder merged commit d108f30 into icsharpcode:master May 7, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Thanks!

@mrmonday mrmonday deleted the mrmonday:308-string-comparisons branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.