Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/)#102218
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
bedevere-bot commentedFeb 24, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit09f2ab6 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
bedevere-bot commentedFeb 24, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit0fef293 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
bedevere-bot commentedFeb 25, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit9b5b62a 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
bedevere-bot commentedFeb 26, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit6378d56 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
bedevere-bot commentedMar 2, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit0863f35 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
bedevere-bot commentedMar 2, 2023
🤖 New build scheduled with the buildbot fleet by@iritkatriel for commit7a436c9 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
All the refleaks tests passed now. |
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.
Nice improvements.
There is some more redundant code that should also be removed (I think).
| PyErr_Fetch(&exc, &val, &tb); | ||
| caught_type =(PyTypeObject *)exc; | ||
| PyObject *exc = PyErr_GetRaisedException(); | ||
| PyTypeObject *caught_type =Py_TYPE(exc); | ||
| /* Ensure type info indicates no extra state is stored at the C level |
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.
Does all this stuff about "wrapping safely" make sense any more?
Since there are no "denomralized" exceptions, this should always work.
I think all this code is making sure that callingPyErr_NormalizeException is safe.
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.
Good question. Let me try to understand what's going on 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.
I don't think this is about PyErr_NormalizeException. This function is called from only one place:
/* Helper that tries to ensure the reported exception chain indicates the * codec that was invoked to trigger the failure without changing the type * of the exception raised. */static voidwrap_codec_error(const char *operation, const char *encoding){ /* TrySetFromCause will replace the active exception with a suitably * updated clone if it can, otherwise it will leave the original * exception alone. */ _PyErr_TrySetFromCause("%s with '%s' codec failed", operation, encoding);}This is a classic use case for notes (PEP 678). It is trying to create an exception of the same type with additional info (and does a lot of stuff to check that it can indeed safely create a new exception of the same type).
I would just rewrite this whole thing to attach the additional info as a note (but not in this PR, it's unrelated).
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.
A PR for that is at:#102407
| /* While we could potentially copy a non-empty instance dictionary | ||
| * to the replacement exception, for now we take the more | ||
| * conservative path of leaving exceptions with attributes set | ||
| * alone. | ||
| */ | ||
| PyErr_Restore(exc, val, tb); | ||
| PyErr_SetRaisedException(exc); | ||
| return NULL; | ||
| } | ||
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 block comment should go as well (if I'm correct about removing the above code)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedMar 3, 2023
When you're done making the requested changes, leave the comment: |
I have made the requested changes; please review again |
bedevere-bot commentedMar 3, 2023
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
* main:pythongh-102304: Consolidate Direct Usage of _Py_RefTotal (pythongh-102514)pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/) (python#102218)pythongh-102507 Remove invisible pagebreak characters (python#102531)pythongh-102515: Remove unused imports in the `Lib/` directory (python#102516) Remove or update bitbucket links (pythonGH-101963)pythongh-101100: Fix sphinx warnings in `zipapp` and `zipfile` modules (python#102526)pythonGH-102397: Fix segfault from race condition in signal handling (python#102399) Fix style in argparse.rst (python#101733) Post 3.12.0a6 fix typo in async generator code field name `ag_code` (python#102448) Python 3.12.0a6
Uh oh!
There was an error while loading.Please reload this page.
(splitting up#102193 to make it more manageable).