-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Move DefaultBinder.CanConvert.cs to shared #23931
Conversation
@@ -1204,6 +1204,63 @@ private static bool CreateParamOrder(int[] paramOrder, ParameterInfo[] pars, str | |||
return true; | |||
} | |||
|
|||
// CanChangePrimitive | |||
// This will determine if the source can be converted to the target type | |||
private static bool CanChangePrimitive(Type source, Type target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in Mono it's internal
and used from Array
code. Can you please check it and make it internal if that's the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks! I hope CoreCLR guys don't mind if use internal here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used from Array code. Can you please check it and make it internal if that's the case?
Is it a good idea to have a dependency from Array to DefaultBinder, TypeCode and rest of reflection? It may be a good idea to implement this using ElementTypes to make this more tree-shaking friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessarily a good idea. It was the only code that implemented primitive type widening check in managed code though (AFAIK). I'd prefer to have the functionality somewhere else and have DefaultBinder
call it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still applies, but it does not prevent merging this.
(source == typeof(UIntPtr) && target == typeof(UIntPtr))) | ||
return true; | ||
|
||
Primitives widerCodes = s_primitiveConversions[(int)(Type.GetTypeCode(source))]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new implementation will have different behavior for enums. The C++ code in DBCanConvertPrimitive
called GetSignatureCorElementType
that returns VALUETYPE for enums, but GetTypeCode
will return the underlying type.
E.g. CanChangePrimitive(typeof(MyEnum), typeof(MyEnum))
returns false today, but it is going to return true with this change.
I think that this should be ok because of none of the callers depend on this, but it would be good if you can double check it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas everywhere where this method is used the second argument (Type) is checked for IsPrimitive
(which is false
for Enum
) so this case is not possible.
b0bb4f1
to
07528df
Compare
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>
Basically it's CoreRT implementation: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/DefaultBinder.CanConvert.cs
I can remove
Primitives
enum and fills_primitiveConversions
with raw values like here.I wrote some relfection-based benchmark, it shows that managed impl is 1-2% faster.
cc @jkotas @marek-safar