[smart_holder] Split out (almost) pure refactoring from PR #5332 #5334
Conversation
PREPARATION for: PR pybind#5332 — Fix handling of const unique_ptr<T, D> & (do not disown). Splitting out so that the functional changes under PR pybind#5332 will be more obvious. The only functional change under this PR is that ``` assert(custom_deleter_ptr != nullptr); ``` is replaced with: ``` if (custom_deleter_ptr == nullptr) { throw std::runtime_error( std::string("smart_holder::extract_deleter() precondition failure (") + context + ")."); } ```
| // 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); |
There was a problem hiding this comment.
Looks like this can be const?
| auto *gd = std::get_deleter<guarded_delete>(vptr); | |
| const auto *gd = std::get_deleter<guarded_delete>(vptr); |
There was a problem hiding this comment.
Good catch, thanks!
Applied here:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
…own). (#5332) * Replace `unique_ptr_cref_roundtrip()` with `pass_unique_ptr_cref()`, `rtrn_unique_ptr_cref()` to make the current behavior obvious. * add in unique_ptr_storage, unique_ptr_storage_deleter * Add shared_ptr_storage (with that disowning fails as expected). * Add load_as_const_unique_ptr() * Restore original struct_smart_holder.h * factor out `smart_holder::extract_deleter()` * Better error message. * Misc cleanup/tidying. * Use `re.match("ctor_arg(_MvCtor)*_MvCtor", ...)` for compatibility with MSVC, NVHPC, ICC * Add small comments. * Fix small, inconsequential oversight in test code. * Apply suggestion by @iwanders under PR #5334 * Remove `std::move()` in `smart_holder::extract_deleter()` * Add `static_assert()` following a suggestion by @iwanders under PR #5334
Description
PREPARATION for:
PR #5332 — Fix handling of
const unique_ptr<T, D> &(do not disown).Splitting out so that the functional changes under PR #5332 will be more obvious.
The only functional change under this PR is that
is replaced with:
@rhaschke FYI
@iwanders FYI (related: #4850, #4921, #4924)
Suggested changelog entry: