Move parts of delegate to shared partition#23552
Conversation
| { | ||
| public abstract partial class Delegate : ICloneable, ISerializable | ||
| { | ||
| internal Delegate() |
There was a problem hiding this comment.
Nit: this file appears to be using tabs in many places... can you change it to use spaces instead?
| @@ -0,0 +1,107 @@ | |||
| using System.Reflection; | |||
| { | ||
| public abstract partial class Delegate : ICloneable, ISerializable | ||
| { | ||
| internal Delegate() |
There was a problem hiding this comment.
Why did this ctor change from private to internal?
There was a problem hiding this comment.
I used the version from corert, it's private now
|
|
||
| public static Delegate Combine(params Delegate[] delegates) | ||
| { | ||
| if (delegates == null || delegates.Length == 0) |
There was a problem hiding this comment.
Nit: it'd be good to be consistent, at least within the same file or least overloads of the same method, about how we do null checks. You changed the above to be a is null... can you change this one to be delegates is null, too?
| return newDelegate; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Why did you remove all of the comments in these operators? They were added very recently, in #21765.
There was a problem hiding this comment.
I worked with several copied and forgot to align this one, should be now identical to coreclr
| public static bool operator ==(Delegate d1, Delegate d2) | ||
| { | ||
| if (d2 is null) | ||
| return d1 is null; |
There was a problem hiding this comment.
Have you validated that the workaround that was employed previously is no longer necessary for performance? The return (d1 is null) ? true : false; was there due to https://github.com/dotnet/coreclr/issues/914, which is still an open issue. Same goes for the changes you made to the return statement below.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool operator !=(Delegate d1, Delegate d2) | ||
| { | ||
| return !(d1 == d2); |
There was a problem hiding this comment.
Did you validate that this doesn't impact perf?
cc: @benaadams
There was a problem hiding this comment.
Used coreclr version but it's missing AggressiveInlining so not sure if it's intentional
There was a problem hiding this comment.
@benaadams Was it intentional to mark operator== with AggressiveInlining, but not operator!= ?
There was a problem hiding this comment.
It was intentional to mark operator== with AggressiveInlining.
It would make sense to mirror it and mark operator!= with AggressiveInlining.
There was a problem hiding this comment.
Why did such small methods not get inlined without the attribute?
There was a problem hiding this comment.
This is not a small method after incorporating the other CR feedback.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool operator ==(Delegate d1, Delegate d2) | ||
| { | ||
| if (d2 is null) |
There was a problem hiding this comment.
This is missing micro-optimizations and related comments from the original version.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool operator !=(Delegate d1, Delegate d2) | ||
| { | ||
| return !(d1 == d2); |
|
@jkotas anything else to be done here? |
jkotas
left a comment
jkotas
left a comment
There was a problem hiding this comment.
LGTM. This just dropped from my radar.
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
5 participants
No description provided.