bpo-35476: clear error in _imp_create_dynamic_impl #14738
Conversation
|
|
||
| mod = _PyImport_FindExtensionObject(name, path); | ||
| if (mod != NULL || PyErr_Occurred()) { | ||
| if (PyErr_Occurred()) { |
There was a problem hiding this comment.
To avoid the overhead of calling PyErr_Occurred(), could you store the result in a variable and check that?
brettcannon
left a comment
There was a problem hiding this comment.
I don't think this is quite doing what @ericsnowcurrently meant when he said "the error should be cleared" as an option in the issue. Instead, I believe what he was thinking was that there be an elif (PyErr_Occurred()) clause to go after the if (mod != NULL) which then clears the exception.
| if (mod != NULL || PyErr_Occurred()) { | ||
| if (PyErr_Occurred()) { | ||
| PyErr_Clear(); | ||
| } |
There was a problem hiding this comment.
This actually won't work because since you cleared out the error but are returning NULL you will trigger a failure in the interpreter for saying there's an exception when one isn't set.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I'm going to kick off a discussion over on the issue which I think may change the approach this PR takes. |
| mod = _PyImport_FindExtensionObject(name, path); | ||
| if (mod != NULL || PyErr_Occurred()) { | ||
| if (PyErr_Occurred()) { | ||
| PyErr_Clear(); |
There was a problem hiding this comment.
I've started a discussion over on the issue where if I'm right and swallowing an exception is bad then this clearing of the exception should be removed.
|
@isidentical please note the conversation on the issue is advocating for explicit exception raising, so that changes the approach you've taken here. |
|
CLosing as @ericsnowcurrently pointed out the current semantics cover the bug. |
clear error in _imp_create_dynamic_impl
https://bugs.python.org/issue35476