- Notifications
You must be signed in to change notification settings - Fork749
Add test for enum int conversion#1811
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/runtime/Util/OpsHelper.cs Outdated
| [ForbidPythonThreads] | ||
| #pragma warning disable IDE1006 // Naming Styles - must match Python | ||
| public static PyInt __int__(T value) | ||
| public static PyInt __int__(T 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.
The[ForbidPythonThreads] attribute should be ensuring that when this method is called, the calling thread holds GIL.
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 the other way round, right? If this attribute is not given, the GIL is explicitly released. If for some reason it has been released before, it will not be taken 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.
The point is: who and why releases GIL when this is called from Python?
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.
That's a great question, but in the end, the code here is wrong asForbidPythonThreads does not take the GIL, right? After this one is merged, I'd declare the current master rc1 and start off a release-3.0 branch, ok?
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.
@filmor you should take this fix instead. I actually addressed ignoringForbidPythonThreads:losttech@025cc5c
What does this implement/fix? Explain your changes.
#1810
Does this close any currently open issues?
#1810
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG