std::unique_ptr deleter unit tests. #26
Closed
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_called
std::function
deleter is not invoked when moved from a unique pointer into the smart holder.from_unique_ptr_with_std_function_deleter+as_raw_ptr_release_ownership
std::function
deleter cannot be disowned.Then I saw the
unique_ptr -> holder -> unique_ptr
roundtrip 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_deleter1
std::function
deleter survives the roundtrip.Problem is, that third unit tests failed... because the
as_unique_ptr
method 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.