Skip to content

Shared Pointers and Protected Destructors #1178

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

Open
varun-intel opened this issue Nov 9, 2017 · 8 comments · May be fixed by #2067
Open

Shared Pointers and Protected Destructors #1178

varun-intel opened this issue Nov 9, 2017 · 8 comments · May be fixed by #2067

Comments

@varun-intel
Copy link

varun-intel commented Nov 9, 2017

I am trying to wrap a C++ class of the following type. The class has a protected destructor but should be containable in a shared_ptr due to the public inheritance from enable_shared_from_this. However, the pybind code produces an error becuase of the protected destructor.

The code also throws an error if I try to use the following pybind wrapper:
py::class_<MyClass, std::unique_ptr<MyClass, py::nodelete>> clsMyClass(mod, "MyClass");

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

namespace py = pybind11;
namespace {

class MyClass : public std::enable_shared_from_this<MyClass> {
protected:
	MyClass() {}
	~MyClass() {}
};

PYBIND11_MODULE(myclass, mod) {
    py::class_<MyClass, std::shared_ptr<MyClass>> clsMyClass(mod, "MyClass");
}
}
@varun-intel varun-intel changed the title Shared Pointers and Protected Constructors Shared Pointers and Protected Destructors Nov 9, 2017
@jagerman
Copy link
Member

The main issue here is that std::shared_ptr isn't usable with a class that doesn't have a public destructor. AFAICS enable_shared_from_this won't help with that; the core issue is that std::shared_ptr will have a delete somewhere in it as an implementation detail, and it's that delete that fails.

For example, the following (without pybind at all) doesn't compile:

#include <memory>

class MyClass : public std::enable_shared_from_this<MyClass> {
public:
    MyClass() {}
protected:
    ~MyClass() {}
};

int main() {
    auto c = std::make_shared<MyClass>();
}

I can't see how enable_shared_from_this would help: its shared_from_this() method isn't usable before a shared_ptr has been constructed around the object in the first place.

You'll likely have to implement your own smart pointer class which handles shared ownership and deletion via some other means.

@varun-intel
Copy link
Author

In this case, MyClass will be treated as an abstract class -- we only want to construct instances of its subtypes, all of which have public destructors. However, we still want to expose the inheritance hierarchy and make use of common methods that are defined in MyClass.

Wouldn't it be possible to avoid generating a line that creates a shared pointer wrapper around MyClass (like std::make_shared()) and only do this for the subclasses?

@varun-intel
Copy link
Author

Thanks for your help btw. I understand that there is an inherent problem (independent of pybind) when you compile a line creating a shared pointer of MyClass. I'm just hoping that the error can be avoided when MyClass is used as an abstract class.

jagerman added a commit to jagerman/pybind11 that referenced this issue Nov 10, 2017
Fixes pybind#1178

It's possible for a non-destructible base class to be declared; such a
holder *can* be obtained via `std::enable_shared_from_this`, but a
shared_ptr cannot be constructed from a returned pointer.

This commit puts the holder constructor behind a `std::is_destructible`
check, giving a runtime failure if we are ever given such a pointer
without a valid `shared_from_this()` shared pointer.
@jagerman
Copy link
Member

jagerman commented Nov 10, 2017

Ah, I see the issue.

Take a look at jagerman@53be819 - I think that should fix it, but feedback is welcome.

@varun-intel
Copy link
Author

Thanks! that seems to work.

@varun-intel
Copy link
Author

Could you perhaps merge this into master?

@varun-intel
Copy link
Author

Reminder, any chance we could merge this in? If not, would you mind if we submit a pull request based on your code?

@jagerman
Copy link
Member

Yes, by all means submit it as a PR. (It also needs a test case added to the test suite to verify that it fixes what it's supposed to fix across the different compilers, but that should be fairly straightforward).

@pd-robgee pd-robgee linked a pull request Jan 13, 2020 that will close this issue
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 a pull request may close this issue.

2 participants