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

[FEAT] Make enable_shared_from_this logic in custom holder types configurable #2957

Open
ezyang opened this issue Apr 16, 2021 · 6 comments
Open

Comments

@ezyang
Copy link

ezyang commented Apr 16, 2021

When defining a holder type that can be initialized from a raw pointer T* (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html), pybind11 conventionally assumes that there is a constructor Holder(T*) which will let it steal a raw pointer reference to initialize into the older. However, sometimes, you want to define a holder type where the raw pointer constructor doesn't have the correct semantics (e.g., std::shared_ptr) or you want to omit the raw pointer constructor and explicitly require users to say if they want to steal or borrow from the passed in raw pointer.

Unfortunately, there is no way to currently do this; you must define another Wrapper class to work around this problem. Here is an example from our codebase, where c10::intrusive_ptr is a holder type but it doesn't support construction from raw pointer (instead it's a static method called reclaim) and so I have to write this boilerplate:

template <typename T>
class IntrusivePtrOkForPybind11 {
  c10::intrusive_ptr<T> impl_;
public:
  IntrusivePtrOkForPybind11() = default;
  IntrusivePtrOkForPybind11(const IntrusivePtrNoGilDestructor&) = default;
  IntrusivePtrOkForPybind11(IntrusivePtrNoGilDestructor&&) = default;
  IntrusivePtrOkForPybind11& operator=(const IntrusivePtrOkForPybind11&) = default;
  IntrusivePtrOkForPybind11& operator=(IntrusivePtrOkForPybind11&&) = default;
  IntrusivePtrOkForPybind11(c10::intrusive_ptr<T> impl) : impl_(std::move(impl)) {}
  IntrusivePtrOkForPybind11(T* impl) : impl_(c10::intrusive_ptr<T>::reclaim(impl)) {}
  T& operator*() const noexcept { return *impl_; }
  T* operator->() const noexcept { return impl_.get(); }
  T* get() const noexcept { return impl_.get(); }
  void reset() noexcept { impl_.reset(); }
  operator bool() const noexcept {
    return impl_;
  }
};

It would be nice if there was some way to configure similar logic to enable_shared_from_this for our own type so we can bypass the raw pointer constructor entirely.

@ezyang
Copy link
Author

ezyang commented Apr 16, 2021

Also related #2067

@EricCousineau-TRI
Copy link
Collaborator

\cc @rwgk if this is something you have encountered during smart_holder investigations?

@ezyang
Copy link
Author

ezyang commented Apr 19, 2021

NB: The example above is subtly wrong because our c10::intrusive_ptr initializes the intrusive refcounts to ZERO not ONE, so we actually have to bump the refcount here, but other implementations (including unique_ptr) might not have done it this way. Buyer beware!

@rwgk
Copy link
Collaborator

rwgk commented Apr 19, 2021

\cc @rwgk if this is something you have encountered during smart_holder investigations?

uhm ... yes, I looked at the existing code to learn what I need to do in the smart_holder code, but it didn't click TBH. I just left it out waiting to come across something that forces me to handle enable_shared_from_this, but that hasn't happened yet, although I'm generally really far in at this point.

Regarding custom smart-pointers in general, quoting from the current README_smart_holder.rst:

  • Caveat (#HelpAppreciated): currently the smart_holder branch does not have a well-lit path for including interoperability with custom smart-pointers. It is expected to be a fairly obvious extension of the smart_holder implementation, but will depend on the exact specifications of each custom smart-pointer type (generalizations are very likely possible).

@rwgk
Copy link
Collaborator

rwgk commented Jun 15, 2021

FYI: I'm working on #3023 to make smart_holder place nice with enable_shared_from_this.

Maybe when I'm done we could review how something like intrusive_ptr could be supported.

@ezyang
Copy link
Author

ezyang commented Jun 16, 2021

Cool. Intrusive_ptr isn't very complicated so I suspect most reasonable designs will work

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

No branches or pull requests

3 participants