std::unique_ptr deleter unit tests.#26
Closed
iwanders wants to merge 3 commits intorwgk:unique_ptr_deleter_shfrom
Closed
std::unique_ptr deleter unit tests.#26iwanders wants to merge 3 commits intorwgk:unique_ptr_deleter_shfrom
iwanders wants to merge 3 commits intorwgk:unique_ptr_deleter_shfrom
Conversation
Last unit test shows that std::function doesn't survive a roundtrip.
iwanders
commented
Nov 1, 2023
| endif() | ||
|
|
||
| add_executable(smart_holder_poc_test smart_holder_poc_test.cpp) | ||
| target_compile_options(smart_holder_poc_test PRIVATE "-g") |
Author
There was a problem hiding this comment.
This obviously needs to go... I needed it to debug something.
Author
|
Closing based on discussion in pybind#4850 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Hello again @rwgk , I saw your call for unit test help in the pybind#2839 (comment) issue (long time watcher on that issue ;) ), I had some time to spare so I took a stab at your unit test request.
So I wasn't too familiar with the original issue, but from the looks of it the main thing is that we need to be able to handle and test custom deleters that are a closure are called at the correct times, only when the delete actually happens.
I tried to think some unit tests around this and came up with these two:
from_unique_ptr_with_std_function_deleter+as_lvalue_ref+destructor_calledstd::functiondeleter is not invoked when moved from a unique pointer into the smart holder.from_unique_ptr_with_std_function_deleter+as_raw_ptr_release_ownershipstd::functiondeleter cannot be disowned.Then I saw the
unique_ptr -> holder -> unique_ptrroundtrip unit test... and realised that the deleter should also survive that roundtrip, so I added;from_unique_ptr_std_function_with_deleter+as_unique_ptr_with_std_function_deleter1std::functiondeleter survives the roundtrip.Problem is, that third unit tests failed... because the
as_unique_ptrmethod on the smart holder didn't extract the deleter from the shared pointer to pass it on to the returnedunique_ptr. In 1ce7053 and 84033fd I tried to remedy this issue, because I think we should pass along the deleter if we go from a smart holder back to a unique pointer.Unfortunately, the unit tests with:
Made this a bit problematic, as that made it impossible to just do an assign into the deleter type of the unique pointer, so there's a tiny trait system here to ensure the assign only happens if the type is actually assignable (preventing compilation errors with the
hld.as_unique_ptr<int, helpers::functor_other_delete<int>>()unit tests).Upon writing this, I think we should actually throw if we have a deleter but are using a return type that cannot accomodate our custom deleter.
Either way, this PR is definitely not ready, I expect formatters & linters will complain about many things, but thought I'd file it anyway to open discussion.