Move DefaultBinder.CanConvert.cs to shared #23931
Conversation
|
|
||
| // 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.
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.
Indeed, thanks! I hope CoreCLR guys don't mind if use internal here
There was a problem hiding this comment.
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.
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.
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.
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.
@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.
EgorBo
force-pushed
the
move-default-binder-to-shared
branch
2 times, most recently
from
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>
3 participants
Basically it's CoreRT implementation: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/DefaultBinder.CanConvert.cs
I can remove
Primitivesenum and fills_primitiveConversionswith raw values like here.I wrote some relfection-based benchmark, it shows that managed impl is 1-2% faster.
cc @jkotas @marek-safar