Skip to content

chore: Delete copy ctor/assign for GIL RAIIs #4183

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

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

Skylion007
Copy link
Collaborator

Description

I noticed some of the GIL RAIIs still had their potential copy assignment operators so I made sure they were explicitly deleted. This should prevent bugprone behavior of trying to copy them / move them around.

Suggested changelog entry:

* GIL RAII scopes are non-copyable to avoid potential bugs.

@Skylion007 Skylion007 requested review from henryiii and rwgk September 16, 2022 19:50
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.

Nice!
It might be good to add the two lines to gil_scoped_acquire_local in detail/internals.h as well. Not that it matters for the code as is, but these kind of fragments tend to get copied around. Having it complete there as well, the oversight/omission won't spread.

@Skylion007 Skylion007 marked this pull request as ready for review September 19, 2022 16:06
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.

Thanks!

@Skylion007 Skylion007 merged commit 9c04c7b into pybind:master Sep 19, 2022
@Skylion007 Skylion007 deleted the gil-delete-spec-members branch September 19, 2022 16:56
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 19, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 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