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

chore: add err guard to capsule destructor and add a move to iostream #3958

Merged
merged 8 commits into from
May 27, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented May 18, 2022

Description

  • One fixes a potential bug in pycapsule destructor by adding an error_guard to one of the dtors.

Suggested changelog entry:

* Avoid potential bug in pycapsule destructor by adding an error_guard to one of the dtors

@Skylion007 Skylion007 requested review from rwgk and henryiii May 18, 2022 14:41
@Skylion007 Skylion007 changed the title chore: add err guard to capsule destructor and remove unused include chore: add err guard to capsule destructor and misc fixes to iostream May 18, 2022
@Skylion007 Skylion007 changed the title chore: add err guard to capsule destructor and misc fixes to iostream chore: add err guard to capsule destructor and add a move to iostream May 18, 2022
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get a chance to look closely today.

I don't fully understand how this plays with the pybind11_fail, but I feel sufficiently confident that this change will only make things better.

It would be good to note somewhere (here or PR description) how you came to add the error_guard (something failed? or did you just happen to notice?)

@Skylion007
Copy link
Collaborator Author

@rwgk I happened to notice that here:

if (ptr == nullptr) {
an error_already_set could be raised for the wrong reason so we should probably enclose the whole thing in an err_scope. I may refactor this a bit more because we already create an err_scope for get_name which is now not necessary. Also, I am wondering if we instead of raising that error, should we just write unraisable like we do in the name_scope.

Thoughts @rwgk ?

@Skylion007 Skylion007 merged commit 68f8010 into pybind:master May 27, 2022
@Skylion007 Skylion007 deleted the skylion007/capsule-dtor-errguard branch May 27, 2022 18:33
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 27, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
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