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 func_handle for rule of two #3169

Merged
merged 2 commits into from
Aug 3, 2021
Merged

Conversation

Skylion007
Copy link
Collaborator

Description

If the copy constructor is defined, the copy assignment constructor should also be defined. Found in #3070 but that was never merged. Found in LGTM static analysis. https://lgtm.com/projects/g/pybind/pybind11/ I looked into turning the clang-tidy check for this, but the only clang-tidy check is way too noisy and will give way too many false positive (only rule of 5 is implemented, ie all constructors, dtor, and assignment operators have to be explicitly defined or defaulted which seem excessive).
The effect is that if we do anything more complex with this struct later, the code will be more maintainable.

Suggested changelog entry:

@Skylion007 Skylion007 requested a review from rwgk August 3, 2021 14:48
@Skylion007 Skylion007 requested a review from henryiii August 3, 2021 14:58
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 think this is a good change to get in asap (incl 2.7.1).

@henryiii
Copy link
Collaborator

henryiii commented Aug 3, 2021

Okay, will backport.

@henryiii henryiii merged commit c0756cc into pybind:master Aug 3, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 3, 2021
henryiii pushed a commit that referenced this pull request Aug 3, 2021
* Fix func_handle for rule of two

* Apply reviewer suggestion
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 3, 2021
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