-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add support for nested C++11 exceptions #3608
Add support for nested C++11 exceptions #3608
Conversation
Seems to be mostly working, but MSVC is unhappy, and possibly there is a separate issue for Python 2. Which reminds me: let's get rid of Python 2 asap! Are you looking at the MSVC errors already? |
@rwgk The python2 failures are missing skipif, I am less sure about MSVC, but I would like to remove |
@rwgk We are all green and I further optimized and simplified the code. |
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.
Cool!
std::rethrow_exception(p); | ||
} catch (error_already_set &e) { | ||
e.restore(); | ||
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.
Are the many return;
needed (to make some compiler(s) happy)?
Would be nice to get rid of them. Or add a comment (maybe) right before the try
, something like:
// The returns inside the catch blocks are needed to silence compiler warnings.
(If that's true.)
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 know why the return's are here. There were here before the diff.
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.
After the missing comment (or however that is resolved), this is good by me.
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 want to look more, but don't let that stop you. Unfortunately I have only super thin time slices at the moment.
}; | ||
|
||
// forward decl | ||
inline void translate_exception(std::exception_ptr); |
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.)
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.
C99 requires the inline on the definition;
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 inline
in 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 inline
based 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 :-)
Over night I ran this through the Google global testing. There were no problems. For completeness: I tested this PR in combination with the smart_holder git-merge-master helper PR #3613. |
Yay! |
Description
Suggested changelog entry: