Skip to content

Fix confusing weakref constructor overload #2832

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 5 commits into from
Jan 31, 2021

Conversation

YannickJadoul
Copy link
Collaborator

Description

Fixes #2536, where the two weakref constructors from handle and object do different things.
Demonstrated by failing tests in the first commit, fixed in the second one.

Suggested changelog entry:

Fix the ``weakref`` constructor from ``py::object`` to create a new ``weakref`` on conversion.

@YannickJadoul YannickJadoul added this to the v2.7 milestone Jan 29, 2021
@rwgk
Copy link
Collaborator

rwgk commented Jan 30, 2021

Looks good to me. I'll globally test this (without the default constructor) to see if anyone is relying on it, mostly out of interest. Preserving the default constructor seems best irrespectively.

@YannickJadoul YannickJadoul changed the title Fix weakref confusing weakref constructor overload Fix confusing weakref constructor overload Jan 30, 2021
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.

Google-global testing this as-is (without the default constructor) came back green. Might be nice to add something along the lines of "very unlikely to be useful but supplied for backward compatibility" in a comment when you put back the default constructor.

@YannickJadoul
Copy link
Collaborator Author

Might be nice to add something along the lines of "very unlikely to be useful but supplied for backward compatibility" in a comment when you put back the default constructor.

If we want to do that, I think we should go with an official deprecation, but I'm fine just being fully backward compatible and pushing the "let's make all py::object subclasses consistent" into the future.

@YannickJadoul
Copy link
Collaborator Author

you put back the default constructor.

I added PYBIND11_OBJECT_CVT_DEFAULT to make clear that this is part of the whole PYBIND11_OBJECT* macro zoo. This groups things if we ever were to e.g. deprecate the default contructors of py::object subclasses, in some hypothetical future.

@YannickJadoul
Copy link
Collaborator Author

I think we went over all bits and pieces, then :-) Let's close another PR and issue.

Thanks, all!

@YannickJadoul YannickJadoul merged commit 6cf6bf2 into pybind:master Jan 31, 2021
@YannickJadoul YannickJadoul deleted the weakref-ctr-overload branch January 31, 2021 22:13
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 31, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 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.

[BUG] Confusing weakref constructor copy the object instead of creating a weakref
4 participants