-
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
Fix: register_exception<>() fails after reinitialization of interpreter #2017
base: master
Are you sure you want to change the base?
Conversation
See [1] for reference. [1] pybind#1610
This fixes pybind#1610 [1]. See previous commit for test cases and [1]. [1] pybind#1610
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.
@rear1019 Thanks for the pull request and sorry for such a late response.
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.
The question in general is if it's a good idea to reinitialize the interpreter; Python's docs at least warn it's not a great idea.
But now we have some tests, and (if the remarks are handled) I don't see much of a problem with including this, so why not.
if (hasattr(scope, name)) | ||
pybind11_fail("Error during initialization: multiple incompatible " | ||
"definitions with name \"" + std::string(name) + "\""); | ||
scope.attr(name) = ex; |
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.
Side-effect: this also allows to register the same exception in multiple scopes. Do we want that? Isn't it better to error on this and tell the user to manually write scope.attr(name) = ex;
?
Meanwhile, ex
's scope will still refer to the old object from before the re-initialization, no? Is this safe?
@@ -282,3 +282,55 @@ TEST_CASE("Reload module from file") { | |||
result = module.attr("test")().cast<int>(); | |||
REQUIRE(result == 2); | |||
} | |||
|
|||
namespace rerun_register_exception |
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.
Very minor detail: do we need this namespace here? Why did you add it?
It might be good practice, but it's also not used for the other tests, so maybe consistency is more important?
See issue #1610 for reference. Test cases for issues described therein are included.