-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[smart_holder] Bug fix: std::unique_ptr
deleter needs to be copied.
#4850
Changes from all commits
f14846d
24bf40b
884fe1c
435d386
17098eb
6083f8a
d815d7d
4dbe657
c684f6c
a685d57
8a973d4
4fb7d12
4d9d722
7669a5b
fe59369
821e13a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#include <pybind11/smart_holder.h> | ||
|
||
#include "pybind11_tests.h" | ||
|
||
#include <memory> | ||
|
||
namespace pybind11_tests { | ||
namespace class_sh_unique_ptr_custom_deleter { | ||
|
||
// Reduced from a PyCLIF use case in the wild by @wangxf123456. | ||
class Pet { | ||
public: | ||
using Ptr = std::unique_ptr<Pet, std::function<void(Pet *)>>; | ||
|
||
std::string name; | ||
|
||
static Ptr New(const std::string &name) { | ||
return Ptr(new Pet(name), std::default_delete<Pet>()); | ||
} | ||
|
||
private: | ||
explicit Pet(const std::string &name) : name(name) {} | ||
}; | ||
|
||
} // namespace class_sh_unique_ptr_custom_deleter | ||
} // namespace pybind11_tests | ||
|
||
PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_unique_ptr_custom_deleter::Pet) | ||
|
||
namespace pybind11_tests { | ||
namespace class_sh_unique_ptr_custom_deleter { | ||
|
||
TEST_SUBMODULE(class_sh_unique_ptr_custom_deleter, m) { | ||
py::classh<Pet>(m, "Pet").def_readwrite("name", &Pet::name); | ||
|
||
m.def("create", &Pet::New); | ||
} | ||
|
||
} // namespace class_sh_unique_ptr_custom_deleter | ||
} // namespace pybind11_tests |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from pybind11_tests import class_sh_unique_ptr_custom_deleter as m | ||
|
||
|
||
def test_create(): | ||
pet = m.create("abc") | ||
assert pet.name == "abc" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This unit test doesn't actually test the deleter is ran, unless ran inside valgrind / lsan, is that intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@wangxf123456 was completely focused on reducing a test from the wild, just enough to reproduce the error message we saw. Copy-pasting what @iwanders wrote in another comment:
I agree. I was also completely focused on the issue in the wild. — Reporting a thought/uncertainty in the back of my mind: Do we actually have to solve this (is that feature actually needed in the wild)? Or would it be sufficient to clearly report an error if someone tried to use the non-existing feature? And then work on it as needed. I'll add a note about it to the PR description, then merge this PR when I see that the GHA are happy.
I really appreciate any help! And I'm a big believer in small steps. I want to encourage you to pick out one thing at a time. Solve it. Merge. Next one. (My goal, in general, is to review PRs within hours; highest priority.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, tricky, it's a balance. I'd at least throw in case there's a non default deleter detected in Speaking of
Sounds good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Feel free to ignore) This line missing a semicolon trips up the syntax highlighting on github, but it seems to be in line with the other tests. Could add a
do {} while(0)
at the end of the macro to enforce the;
at the end of the line. But I think that requires a lot of changes in other unit tests as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Too late", unfortunately: I just looked, I'd have to change 35 files around the Google codebase, outside the pybind11 source directory. It'll set me back a whole day or two worth of effort. And there are so many other far more substantial things wishing for those days.