-
Notifications
You must be signed in to change notification settings - Fork 295
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
Fix conversions in _SimpleCData
#1912
Conversation
__index__
over __int__
in ModuleOps_SimpleCData
It started as simply preferring |
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.
Not super familiar with this part of the codebase, but looks good to me.
if (value is BigInteger) { | ||
return (long)(BigInteger)value; | ||
if (PythonOps.TryToInt(value, out BigInteger bi)) { | ||
return unchecked((long)(ulong)(bi & ulong.MaxValue)); |
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 guess this will no longer throw if the value is too big for a long
. Not too familiar with these methods, but just confirming that this is the intended behaviour?
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 guess test_conversions_overflow
answers my question...
@@ -709,10 +710,13 @@ void INativeType.EmitReverseMarshalling(ILGenerator method, LocalOrArg value, Li | |||
} | |||
} | |||
|
|||
private static void EmitInt64ToObject(ILGenerator method, LocalOrArg value) { | |||
private static void EmitXInt64ToObject(ILGenerator method, LocalOrArg value) { |
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.
What does X
stand for? 😄
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.
Wildcard *
😄
Besides handling
__index__
, it also better handlesBigInteger
values, which are now Pythonint
. This probably should have been done as part of #1329, but at that time I wasn't aware of this corner of the codebase.Also, there is a bunch of int/index conversions in
Converter
that partially overlap the conversions inPythonOps
. I wonder it they could be consolidated somehow.