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

Enable defining custom __new__ #3265

Merged
merged 8 commits into from
Sep 20, 2021

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Sep 11, 2021

Description

Suggested changelog entry:

* Enable defining custom ``__new__`` methods on classes by fixing bug preventing overriding methods if they have non-pybind11 siblings.

@Skylion007 Skylion007 requested a review from rwgk September 12, 2021 00:20
…urning pointer from "__init__" instead of reference.
@rwgk
Copy link
Collaborator

rwgk commented Sep 12, 2021

How did I arrive at 194033e?

When testing in the Google environment, I ran into this assert:

https://github.com/python/cpython/blob/d5650a1738fe34f6e1db4af5f4c4edb7cae90a36/Objects/typeobject.c#L3117

The error wasn't obvious at all, therefore I inserted this right before that assert:

    if (PyErr_Occurred()) {                                                     
      PyErr_Print();                                                            
      assert(0);                                                                
    } 

That way I got to see the FutureWarning. After disabling it with the unconditional #define, all tests ran ASAN clean.

While looking around, I saw that all new-style "__init__" return a pointer (from a placement new). Therefore I changed the new code accordingly, before I discovered the FutureWarning. I didn't go back to test if it also works when returning a reference (I believe returning a pointer is preferable regardless, although I don't have deep insights).

That's all I know. 🙂

@Skylion007 Skylion007 requested a review from henryiii September 12, 2021 14:59
@Skylion007
Copy link
Collaborator Author

Defining a custom new function for enums could help close #1177. Also we are putting a class method in the tests now so we might want to add a class method shim as mentioned in #1693 now to py_class as the current way of defining class methods is bulky and not very intuitive.

We don't seem to have a way to check the py::type of a the C++ class for the first argument though, which is a bit annoying.

@Skylion007
Copy link
Collaborator Author

This syntax is at the very least not terrible until we have proper classmethod support.

@Skylion007 Skylion007 requested a review from henryiii September 17, 2021 21:40
@Skylion007 Skylion007 merged commit d0f3c51 into pybind:master Sep 20, 2021
@Skylion007 Skylion007 deleted the enable_defining__new__ branch September 20, 2021 14:42
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 20, 2021
@Skylion007
Copy link
Collaborator Author

I actually just realized those PTRs in test should probably be ref since they can't be null. Probably can go back and fix them when we write the actual documentation for this feature at some point @henryiii along with proper classmethod support perhaps


py::class_<NoConstructor>(m, "NoConstructor")
.def_static("new_instance", &NoConstructor::new_instance, "Return an instance");

py::class_<NoConstructorNew>(m, "NoConstructorNew")
.def(py::init([](NoConstructorNew *self) { return self; })) // Need a NOOP __init__
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also wish there was a cleaner way to not have to implement __init__ if new is defined, but I can't figure out how to hack to the metaclass not to complain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something like .def_new(...) or .def(py::new(...)) that wraps the two methods you have right now?

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 24, 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