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.

Sign up for GitHub

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

Use weakref to clean up captured function object in def_buffer #2634

Merged
merged 1 commit into from
Nov 2, 2020
Merged

Use weakref to clean up captured function object in def_buffer #2634

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,8 @@ class class_ : public detail::generic_type {
return *this;
}

template <typename Func> class_& def_buffer(Func &&func) {
template <typename Func>
class_& def_buffer(Func &&func) {
struct capture { Func func; };
auto *ptr = new capture { std::forward<Func>(func) };
install_buffer_funcs([](PyObject *obj, void *ptr) -> buffer_info* {
Expand All @@ -1324,6 +1325,10 @@ class class_ : public detail::generic_type {
return nullptr;
return new buffer_info(((capture *) ptr)->func(caster));
}, ptr);
weakref(m_ptr, cpp_function([ptr](handle wr) {
delete ptr;
wr.dec_ref();
})).release();
Comment on lines +1328 to +1331
Copy link
Collaborator Author

@YannickJadoul YannickJadoul Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm thinking we could still refactor this to something like

template <typename F>
void cleanup_through_weakref(handle parent, F &&f) {
    weakref(parent, cpp_function([f=std::forward<F>(f)](handle wr) {
        f();
        wr.dec_ref();
    })).release();
};

But we'd also need some workaround for C++11's inability to capture-by-move. So let's maybe leave this for a next PR, if this pattern keeps coming back.

return *this;
}

Expand Down