Skip to content
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

Merged
merged 4 commits into from
Feb 17, 2025
Merged

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Feb 12, 2025

Besides handling __index__, it also better handles BigInteger values, which are now Python int. 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 in PythonOps. I wonder it they could be consolidated somehow.

@BCSharp BCSharp changed the title Prefer __index__ over __int__ in ModuleOps Fix conversions in _SimpleCData Feb 16, 2025
@BCSharp
Copy link
Member Author

BCSharp commented Feb 16, 2025

It started as simply preferring __index__ over __int__ in ModuleOps, but it grew to encompass all kind of conversion fixes and improvements.

@BCSharp BCSharp marked this pull request as ready for review February 16, 2025 07:02
@BCSharp BCSharp requested a review from slozier February 16, 2025 07:03
Copy link
Contributor

@slozier slozier left a 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));
Copy link
Contributor

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?

Copy link
Contributor

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) {
Copy link
Contributor

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? 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildcard * 😄

@BCSharp BCSharp merged commit 7a3107c into IronLanguages:main Feb 17, 2025
8 checks passed
@BCSharp BCSharp deleted the moduleops__index__ branch February 17, 2025 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants