-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-45635: refactor print_exception_recursive into smaller functions to standardize error handling #30015
Conversation
…to standardize error handling
if (err) { | ||
/* don't do anything else */ | ||
} |
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.
Wouldn't it be clearer to just return if we don't need to do anything else? Actually, seeing that err
is only set twice, I wonder if it could be worth it to just inline the error handling (PyErr_Clear()
) and remove err
🤔 Nitpicking 😎
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 will happen, once this function stops swallowing errors (that's a behaviour change rather than a refactor). Right now we have the PyErr_Clear() at the end.
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
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.
One last PEP 7 comment. Other than that, looks good to me!
https://bugs.python.org/issue45635