-
Notifications
You must be signed in to change notification settings - Fork 822
Needless emit of callvirt for nullcheck when C# doesn't do it in corresponding code #2844
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
Comments
@KevinRansom No offence. |
@Huqin-China The main issue we will track with this bug is the emit of You can suppress this with I wonder if we should really go back to making But what I also really want to know is why we are emitting callvirt at all in a situation where C# doesn't. |
@Huqin-China that post was mostly caused by a major frustration with .NET Core in project.json era and the situation is visibly improving, esp. with F# 4.1 and VS2017. Taking a pause did not hurt, but F# will stay there and is getting better. Remaining issues are either well known and being taken care of, or there is always a workaround with using a C# assembly for low-level imperative things, like in Spreads and Hopac, and this issue is from using that approach. @KevinRansom Thanks for reacting to this so fast. I wanted to create an issue mentioning #1637 with some samples and additional benchmarks. In my case I have a collection that often calls small private methods (which suffer from @dsyme --alwayscallvirt- is a very easy solution, but there is a compiler warning for that flag saying it is for experiments only. Plus with the comments in the issue #468 it doesn't sound like a good idea to apply it to a non-trivial assembly. |
The case for using "callvirt" is explained here: http://v2matveev.blogspot.com/2010/11/null-stringgetenumerator.html Here are some relevant snippets quoted then:
|
Now F# compiler doesn't use the knowledge for case 3 (calling |
This link checks out the code for a relatively simple construct. F# does not necessarily do well.
Spreads/Spreads@b12b3be
The text was updated successfully, but these errors were encountered: