Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rear1019
Copy link

@rear1019 rear1019 commented Dec 5, 2019

See issue #1610 for reference. Test cases for issues described therein are included.

Copy link
Collaborator

@bstaletic bstaletic left a 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.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a 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;
Copy link
Collaborator

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
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants