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

[smart_holder] Split out (almost) pure refactoring from PR #5332 #5334

Merged
merged 1 commit into from
Aug 25, 2024
Merged

[smart_holder] Split out (almost) pure refactoring from PR #5332 #5334

Show file tree
Hide file tree
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
16 changes: 16 additions & 0 deletions include/pybind11/detail/struct_smart_holder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,22 @@ struct smart_holder {
vptr_del_ptr->armed_flag = armed_flag;
}

// Caller is responsible for precondition: ensure_compatible_rtti_uqp_del<T, D>() must succeed.
template <typename T, typename D>
std::unique_ptr<D> extract_deleter(const char *context) const {
auto *gd = std::get_deleter<guarded_delete>(vptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can be const?

Suggested change
auto *gd = std::get_deleter<guarded_delete>(vptr);
const auto *gd = std::get_deleter<guarded_delete>(vptr);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks!

Applied here:

a0be89d

There is actually another detail here that I'm unsure about, a little further down:

            return std::unique_ptr<D>(new D(std::move(custom_deleter_ptr->deleter)));

Is the std::move() there what we want?

Tests pass without it. (I ran local testing only, Linux gcc.)

From looking at

it isn't clear to me if the Deleter is required to be copyable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question! I think it is not required, see the constructor for unique_ptr:

Specifically the notes for the 3,4) signatures:

If D is non-reference type A, then the signatures are:
unique_ptr(pointer p, const A& d) noexcept; (1) (requires that Deleter is nothrow-CopyConstructible)
unique_ptr(pointer p, A&& d) noexcept; (2) (requires that Deleter is nothrow-MoveConstructible)

So I think moving is the correct thing to do in case the second signature was used to create the unique pointer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

Now I'm backtracking to: While it's not a requirement in the ISO standard, should it be a requirement here?

Because we are extracting the deleter multiple times: each time we build a const unique_ptr<T, D> & with the code under #5332.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, well, yeah if we end up extracting the deleter multiple times... we should not be moving it, and probably want a static assert to ensure it is actually copyable?

if (gd && gd->use_del_fun) {
const auto &custom_deleter_ptr = gd->del_fun.template target<custom_deleter<T, D>>();
if (custom_deleter_ptr == nullptr) {
throw std::runtime_error(
std::string("smart_holder::extract_deleter() precondition failure (") + context
+ ").");
}
return std::unique_ptr<D>(new D(std::move(custom_deleter_ptr->deleter)));
}
return nullptr;
}

static smart_holder from_raw_ptr_unowned(void *raw_ptr) {
smart_holder hld;
hld.vptr.reset(raw_ptr, [](void *) {});
Expand Down
17 changes: 1 addition & 16 deletions include/pybind11/detail/type_caster_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,22 +794,7 @@ struct load_helper : value_and_holder_helper {
"instance cannot safely be transferred to C++.");
}

// Temporary variable to store the extracted deleter in.
std::unique_ptr<D> extracted_deleter;

auto *gd = std::get_deleter<pybindit::memory::guarded_delete>(holder().vptr);
if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del<T, D>() call above.
// In struct_smart_holder, a custom deleter is always stored in a guarded delete.
// The guarded delete's std::function<void(void*)> actually points at the
// custom_deleter type, so we can verify it is of the custom deleter type and
// finally extract its deleter.
using custom_deleter_D = pybindit::memory::custom_deleter<T, D>;
const auto &custom_deleter_ptr = gd->del_fun.template target<custom_deleter_D>();
assert(custom_deleter_ptr != nullptr);
// Now that we have confirmed the type of the deleter matches the desired return
// value we can extract the function.
extracted_deleter = std::unique_ptr<D>(new D(std::move(custom_deleter_ptr->deleter)));
}
std::unique_ptr<D> extracted_deleter = holder().template extract_deleter<T, D>(context);

// Critical transfer-of-ownership section. This must stay together.
if (self_life_support != nullptr) {
Expand Down