Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support for nested C++11 exceptions#3608
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
ab9a1ec269da46bf438d942d5833a1ac8414cf2754eb13befb89620a09d1ad53a9156c5145500d2b87f2c745ee3442d958d369c87c49df0d2578c871c526cf65a5ad8e6cbe623fa58eeFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -10,6 +10,7 @@ | ||
| #pragma once | ||
| #include "../pytypes.h" | ||
| #include <exception> | ||
| /// Tracks the `internals` and `type_info` ABI version independent of the main library version. | ||
| /// | ||
| @@ -280,21 +281,104 @@ inline internals **&get_internals_pp() { | ||
| return internals_pp; | ||
| } | ||
| #if PY_VERSION_HEX >= 0x03030000 | ||
| // forward decl | ||
| inline void translate_exception(std::exception_ptr); | ||
| template <class T, | ||
| enable_if_t<std::is_same<std::nested_exception, remove_cvref_t<T>>::value, int> = 0> | ||
| bool handle_nested_exception(const T &exc, const std::exception_ptr &p) { | ||
| std::exception_ptr nested = exc.nested_ptr(); | ||
| if (nested != nullptr && nested != p) { | ||
| translate_exception(nested); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| template <class T, | ||
| enable_if_t<!std::is_same<std::nested_exception, remove_cvref_t<T>>::value, int> = 0> | ||
| bool handle_nested_exception(const T &exc, const std::exception_ptr &p) { | ||
| if (auto *nep = dynamic_cast<const std::nested_exception *>(std::addressof(exc))) { | ||
Skylion007 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return handle_nested_exception(*nep, p); | ||
| } | ||
| return false; | ||
| } | ||
| #else | ||
| template <class T> | ||
| bool handle_nested_exception(const T &, std::exception_ptr &) { | ||
| return false; | ||
| } | ||
| #endif | ||
| inline bool raise_err(PyObject *exc_type, const char *msg) { | ||
| #if PY_VERSION_HEX >= 0x03030000 | ||
| if (PyErr_Occurred()) { | ||
| raise_from(exc_type, msg); | ||
| return true; | ||
| } | ||
| #endif | ||
| PyErr_SetString(exc_type, msg); | ||
| return false; | ||
| }; | ||
| inline void translate_exception(std::exception_ptr p) { | ||
| if (!p) { | ||
| return; | ||
| } | ||
| try { | ||
| if (p) std::rethrow_exception(p); | ||
| } catch (error_already_set &e) { e.restore(); return; | ||
| } catch (const builtin_exception &e) { e.set_error(); return; | ||
| } catch (const std::bad_alloc &e) { PyErr_SetString(PyExc_MemoryError, e.what()); return; | ||
| } catch (const std::domain_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return; | ||
| } catch (const std::invalid_argument &e) { PyErr_SetString(PyExc_ValueError, e.what()); return; | ||
| } catch (const std::length_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return; | ||
| } catch (const std::out_of_range &e) { PyErr_SetString(PyExc_IndexError, e.what()); return; | ||
| } catch (const std::range_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return; | ||
| } catch (const std::overflow_error &e) { PyErr_SetString(PyExc_OverflowError, e.what()); return; | ||
| } catch (const std::exception &e) { PyErr_SetString(PyExc_RuntimeError, e.what()); return; | ||
| std::rethrow_exception(p); | ||
| } catch (error_already_set &e) { | ||
| handle_nested_exception(e, p); | ||
| e.restore(); | ||
Skylion007 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return; | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the many // The returns inside the catch blocks are needed to silence compiler warnings. CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why the return's are here. There were here before the diff. | ||
| } catch (const builtin_exception &e) { | ||
| // Could not use template since it's an abstract class. | ||
| if (auto *nep = dynamic_cast<const std::nested_exception *>(std::addressof(e))) { | ||
| handle_nested_exception(*nep, p); | ||
| } | ||
| e.set_error(); | ||
| return; | ||
| } catch (const std::bad_alloc &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_MemoryError, e.what()); | ||
| return; | ||
| } catch (const std::domain_error &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_ValueError, e.what()); | ||
| return; | ||
| } catch (const std::invalid_argument &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_ValueError, e.what()); | ||
| return; | ||
| } catch (const std::length_error &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_ValueError, e.what()); | ||
| return; | ||
| } catch (const std::out_of_range &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_IndexError, e.what()); | ||
| return; | ||
| } catch (const std::range_error &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_ValueError, e.what()); | ||
| return; | ||
| } catch (const std::overflow_error &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_OverflowError, e.what()); | ||
| return; | ||
| } catch (const std::exception &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_RuntimeError, e.what()); | ||
| return; | ||
| } catch (const std::nested_exception &e) { | ||
| handle_nested_exception(e, p); | ||
| raise_err(PyExc_RuntimeError, "Caught an unknown nested exception!"); | ||
| return; | ||
| } catch (...) { | ||
| PyErr_SetString(PyExc_RuntimeError, "Caught an unknown exception!"); | ||
| raise_err(PyExc_RuntimeError, "Caught an unknown exception!"); | ||
| return; | ||
| } | ||
| } | ||
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.
Forward declarations should never have
inline:https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions
(I find this highly counter-intuitive myself.)
Uh oh!
There was an error while loading. Please reload this page.
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.
C99 requires the inline on the definition; are you sure C++ cares? Looking at the spec, it seems clearly allowed unless it's nested inside another function https://en.cppreference.com/w/cpp/language/inline & https://en.cppreference.com/w/cpp/language/declarations#Specifiers seem to indicate it's allowed, and I didn't have any problems on any complier I quickly checked on godbolt.
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.
Ouch.
My previous comment was based on what I remembered from working on
#3179.
From memory, in a Google-internal chat I was advised to not use
inlinein forward declarations.Am I sure? No. I was asking for advice and then going with that. The work on PR #3179 left me believing that even compilers are unusually confused about what is correct.
To be pragmatic, we can reduce it to: do we want to be consistent within pybind11? PR #3179 removed some
inlinebased on the advice I got, and to be consistent.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.
@rwgk Other forward declare (line 40) still use inline.
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 intentionally kept #3179 limited to functions involving
PYBIND11_NOINLINE. I didn't look around too much, as I very often do, to not let a good project die the death of a thousand cuts.For this PR, idk. If we're inconsistent already and the advice I got in Aug last year isn't universally accepted (as I was thinking until now) ... shrug :-)