Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gh-106672: C API: Report indiscriminately ignored errors #106674
gh-106672: C API: Report indiscriminately ignored errors #106674
Changes from 15 commits
e134bad
00ce038
168f92a
884da66
58dbac5
07c8250
56e3a49
4e13132
b5a661b
ab5eb8f
c681ce2
82efd1c
c687ff8
310ae51
241576a
fd50ea9
587b3cc
bfb206a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe rename to 'item' or '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.
Done.
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.
Can these 3 functions be called with an exception set? They don't override the exception? That sounds surprising. I would prefer suggesting to not call the function with an exception set. What do you think?
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.
Got catch. Indeed,
PyMapping_GetOptionalItemString()
can only return 0 if no exception is set, so this condition is always false. Also, the alternative functions also can clear exceptions.I think that we should classify the C API by classes:
There may be more classes.
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.
In the general case, to write safe code handling a raised exception, I think the safest option is to keep the exception aside using
PyErr_GetRaisedException()
. Maybe today some functions are perfectly fine and never override the currently raised exception. But what if tomorrow their implementation changes, and they may start to clear the currently raised exception?In Python, in an
except:
block, there is no "currently raised exception" in the C level, even ifsys.exc_info()
returns the exception. The difference betweenPyThreadState.exc_info
andPyThreadState.current_exception
is subtle.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.
This is why it should be clearly documented.
Obviously you can call
PyErr_Occurred()
,PyErr_GetRaisedException()
orPyErr_WriteUnraisable()
when an exception is set.