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

Improve handling of bitfields in ctypes #1441

Merged
merged 12 commits into from
May 9, 2022

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented May 7, 2022

Addressed #1439 and a few other issues that caught my attention. The most interesting is a fix for python/cpython#90914, which was reported just 3 months ago and is still not resolved. Funny thought that IronPython on one point will be ahead of CPython. Not that this functionality is used anywhere in the wild, since it is not working yet in CPython.

@BCSharp BCSharp changed the title Improve handling of c_bool bitfields in ctypes Improve handling of bitfields in ctypes May 7, 2022
@BCSharp BCSharp force-pushed the BitField_ctypes branch 2 times, most recently from 22453b3 to 5d07702 Compare May 8, 2022 22:46
@BCSharp BCSharp marked this pull request as ready for review May 8, 2022 23:58
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.

Looks good to me! Good thing you took this on since I almost certainly wouldn't have been as thorough! 😄

Src/IronPython.Modules/_ctypes/Field.cs Show resolved Hide resolved
}
}
} else {
throw PythonOps.TypeErrorForTypeMismatch("int", value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this bit is unreachable so you can throw InvalidOperationException?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. It's a left-over from a pre-3.8 code version.

value = (BigInteger)bits;
}

} else if (value is bool bVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this reachable or bool will be caught by the IsBoolType check above?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not reachable. Again a leftover from a previous version.

@@ -50,6 +50,10 @@ public abstract class _Structure : CData {
INativeType nativeType = NativeType;
StructType st = (StructType)nativeType;

if (args is null) { // None passed as a sole argument
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, None gives null here? Would adding the NotNoneAttribute solve this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, None gives null. This is the consequence that the DLR allows to bind to the param object[] array directly (just like C#). We have discussed it before (IronLanguages/dlr#249). Although then I was arguing against it, I now think that for params arrays it will be better to keep the behaviour as it is now.

But indeed, [NotNone] prevents it from happening, pushing null into the params array. Good insight! It is an elegant solution.

@BCSharp
Copy link
Member Author

BCSharp commented May 9, 2022

Thanks for the review!

@slozier slozier merged commit 9636e35 into IronLanguages:master May 9, 2022
@BCSharp BCSharp deleted the BitField_ctypes branch May 9, 2022 21:41
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