Conversation
ghost
added
needs-area-label
|
Some comments " |
src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs
Outdated
Show resolved
Hide resolved
| // Test d2 first to allow branch elimination when inlined for not null checks (!= null) | ||
| // so it can become a simple test |
There was a problem hiding this comment.
These comments aren't really "correct" anymore
We should likely ensure that the same general codegen and dead code elimination still happens when inlined (for RyuJIT and Mono).
There was a problem hiding this comment.
We should likely ensure that the same general codegen and dead code elimination still happens
+1. The MihuBot report has number of regressions. The regressions seem to be caused by dead code elimination no longer happening - a lot of new System.Object:Equals calls that did not exist before.
There was a problem hiding this comment.
The list of regressions seems to be a good data point about methods that use == null / != null, but that should better be using is null / is not null instead.
There was a problem hiding this comment.
The list of regressions seems to be a good data point about methods that use == null / != null, but that should better be using is null / is not null instead.
Are you interested in a submitting PR that fixes these? (Just in the shipping libraries, tests can and should stay as is.)
There was a problem hiding this comment.
use
== null/!= null, but that should better be usingis null/is not nullinstead.
Are custom .Equals implementations that might return true with a single null (like for example with Unity) not a concern here?
There was a problem hiding this comment.
We can look at it case-by-case. Most of the affected operator implementations check for the null upfront and won't call Equals with null.
Do you have an example of where it may be a problem?
danmoseley
added
area-System.Runtime
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsAs requested in #97959 (comment) cc @jkotas
|
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
5 participants
As requested in #97959 (comment)
cc @jkotas