Skip to content

Change base parameter type in register_exception and exception constructor from PyObject* to handle #2467

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 2 commits into from
Sep 15, 2020

Conversation

YannickJadoul
Copy link
Collaborator

Follow-up of #2465. This should be the same, but is the pybind11 alternative, rather than a raw C API type.

@bstaletic
Copy link
Collaborator

/usr/include/python3.7m/pyerrors.h:326:24: note: candidate function not viable: no known conversion from 'pybind11::handle' to 'PyObject *' (aka '_object *') for 2nd argument

So this is a breaking change. py::handle can not be constructed from a PyObject* without also specifying whether you want to borrow or steal the reference.

On top of that, you mentioned using py::type instead of py::handle?

@YannickJadoul
Copy link
Collaborator Author

/usr/include/python3.7m/pyerrors.h:326:24: note: candidate function not viable: no known conversion from 'pybind11::handle' to 'PyObject *' (aka '_object *') for 2nd argument

So this is a breaking change. py::handle can not be constructed from a PyObject* without also specifying whether you want to borrow or steal the reference.

Huh, I thought you could. handle is only a wrapper around PyObject*, not doing any refcounting (borrow/steal are for object, when the refcount needs to be increased or not).

There's also an implicit conversion, so it's a bit weird that this doesn't work?

handle(PyObject *ptr) : m_ptr(ptr) { } // Allow implicit conversion from PyObject*

On top of that, you mentioned using py::type instead of py::handle?

Yes, but it's not there, yet. So indeed, let's link to #2388.

@YannickJadoul
Copy link
Collaborator Author

There's also an implicit conversion, so it's a bit weird that this doesn't work?

It's the opposite way that doesn't work implicitly, handle to PyObject*. Should've given the thing a compilation run before pushing "a harmless change" ;-)

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Right, I was mixing things up with py::object(py::handle), which doesn't work.

Anyway, this looks good now.

@michalsustr
Copy link
Contributor

michalsustr commented Sep 8, 2020

Does this mean that now it is possible to do:

py::register_exception<ExceptionA>(m, "ExceptionA", PyExc_RuntimeError);
py::register_exception<ExceptionB>(m, "ExceptionB", ExceptionA);

i.e. make a hierarchy of exceptions, starting with python ones and continuing into the ones defined on the C++ side?

If not, how can this be achieved?

@YannickJadoul
Copy link
Collaborator Author

@michalsustr Not sure, actually. I would think it has always worked, but that before, you'd need to say py::register_exception<ExceptionB>(m, "ExceptionB", ExceptionA.ptr());, because handle is not implicitly convertible to a PyObject*.

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Sep 8, 2020

@michalsustr (and others), I quickly checked, and this seems to already work:

#include <pybind11/pybind11.h>

namespace py = pybind11;

class X : public std::runtime_error {};
class Y : public std::runtime_error {};

PYBIND11_MODULE(example, m)
{
        auto exceptionA = py::register_exception<X>(m, "ExceptionA", PyExc_RuntimeError);
        py::register_exception<Y>(m, "ExceptionB", exceptionA.ptr());
}

So this PR just removes the need to add .ptr(), but that's all. So this PR shouldn't break/enable anything more?
It would of course still be nice to unify with py::class_, but that's a whole other expedition into unknown territory.

@henryiii henryiii changed the title Change base parameter type in register_exception and excepion constructor from PyObject* to handle Change base parameter type in register_exception and exception constructor from PyObject* to handle Sep 14, 2020
@henryiii henryiii merged commit 16f199f into pybind:master Sep 15, 2020
@YannickJadoul YannickJadoul deleted the exception-base-handle branch September 15, 2020 14:32
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.

4 participants