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

std::unique_ptr deleter unit tests. #26

Conversation

iwanders
Copy link

@iwanders iwanders commented Nov 1, 2023

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
    • Checks that std::function deleter is not invoked when moved from a unique pointer into the smart holder.
    • Verifies the deleter is invoked when the smart holder is destructed.
  • from_unique_ptr_with_std_function_deleter+as_raw_ptr_release_ownership
    • Checks that anything with a 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
    • Confirms that the 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 returned unique_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:

    REQUIRE_THROWS_WITH((hld.as_unique_ptr<int, helpers::functor_other_delete<int>>()),
                        "Incompatible unique_ptr deleter (as_unique_ptr).");

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.

@@ -9,6 +9,7 @@ else()
endif()

add_executable(smart_holder_poc_test smart_holder_poc_test.cpp)
target_compile_options(smart_holder_poc_test PRIVATE "-g")
Copy link
Author

Choose a reason for hiding this comment

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

This obviously needs to go... I needed it to debug something.

@iwanders
Copy link
Author

iwanders commented Nov 3, 2023

Closing based on discussion in pybind#4850

@iwanders iwanders closed this Nov 3, 2023
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

Successfully merging this pull request may close these issues.

1 participant