-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add py::potentially_slicing_weak_ptr(handle)
function
#5624
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
Changes from all commits
50c3c1e
528d1cc
b160f6c
2fe82b2
ff70657
a481647
32858da
e978d93
1c0b700
4638e01
6cc6368
62d3265
2f672eb
888a295
b20f557
3b4b28c
56d23dc
ff0792e
eb1787c
10f6e78
121c8cf
b42bd11
f160450
78178d7
62d2242
0ecb396
faf1e61
222b4a7
7c4a8bc
03a8981
bad0c12
683fff4
9f379ea
b349e84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -759,7 +759,11 @@ struct load_helper : value_and_holder_helper { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
std::shared_ptr<T> load_as_shared_ptr(void *void_raw_ptr, | ||||||||||||||||||
handle responsible_parent = nullptr) const { | ||||||||||||||||||
handle responsible_parent = nullptr, | ||||||||||||||||||
// to support py::potentially_slicing_weak_ptr | ||||||||||||||||||
// with minimal added code complexity: | ||||||||||||||||||
bool force_potentially_slicing_shared_ptr | ||||||||||||||||||
= false) const { | ||||||||||||||||||
if (!have_holder()) { | ||||||||||||||||||
return nullptr; | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -774,7 +778,7 @@ struct load_helper : value_and_holder_helper { | |||||||||||||||||
throw std::runtime_error("Non-owning holder (load_as_shared_ptr)."); | ||||||||||||||||||
} | ||||||||||||||||||
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a comment here to explain the purpose of the new parameter 'force_potentially_slicing_shared_ptr' and how it affects the alias check, so future maintainers understand why this conditional branch bypasses the slicing protection.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chose this less verbose, and less redundant, alternative: commit b349e84 |
||||||||||||||||||
if (python_instance_is_alias) { | ||||||||||||||||||
if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) { | ||||||||||||||||||
auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(hld.vptr); | ||||||||||||||||||
if (vptr_gd_ptr != nullptr) { | ||||||||||||||||||
std::shared_ptr<void> released_ptr = vptr_gd_ptr->released_ptr.lock(); | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
// Copyright (c) 2025 The pybind Community. | ||
// All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
#include "pybind11_tests.h" | ||
|
||
#include <memory> | ||
|
||
namespace pybind11_tests { | ||
namespace potentially_slicing_weak_ptr { | ||
|
||
template <int> // Using int as a trick to easily generate multiple types. | ||
struct VirtBase { | ||
virtual ~VirtBase() = default; | ||
virtual int get_code() { return 100; } | ||
}; | ||
|
||
using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder | ||
using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder | ||
|
||
// Similar to trampoline_self_life_support | ||
struct trampoline_is_alive_simple { | ||
std::uint64_t magic_token = 197001010000u; | ||
|
||
trampoline_is_alive_simple() = default; | ||
|
||
~trampoline_is_alive_simple() { magic_token = 20380118191407u; } | ||
|
||
trampoline_is_alive_simple(const trampoline_is_alive_simple &other) = default; | ||
trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept | ||
: magic_token(other.magic_token) { | ||
other.magic_token = 20380118191407u; | ||
} | ||
|
||
trampoline_is_alive_simple &operator=(const trampoline_is_alive_simple &) = delete; | ||
trampoline_is_alive_simple &operator=(trampoline_is_alive_simple &&) = delete; | ||
}; | ||
|
||
template <typename VB> | ||
const char *determine_trampoline_state(const std::shared_ptr<VB> &sp) { | ||
if (!sp) { | ||
return "sp nullptr"; | ||
} | ||
auto *tias = dynamic_cast<trampoline_is_alive_simple *>(sp.get()); | ||
if (!tias) { | ||
return "dynamic_cast failed"; | ||
} | ||
if (tias->magic_token == 197001010000u) { | ||
return "trampoline alive"; | ||
} | ||
if (tias->magic_token == 20380118191407u) { | ||
return "trampoline DEAD"; | ||
} | ||
return "UNDEFINED BEHAVIOR"; | ||
} | ||
|
||
struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support, trampoline_is_alive_simple { | ||
using VirtBaseSH::VirtBaseSH; | ||
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSH, get_code); } | ||
}; | ||
|
||
struct PyVirtBaseSP : VirtBaseSP, trampoline_is_alive_simple { // self-life-support not available | ||
using VirtBaseSP::VirtBaseSP; | ||
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSP, get_code); } | ||
}; | ||
|
||
template <typename VB> | ||
std::shared_ptr<VB> rtrn_obj_cast_shared_ptr(py::handle obj) { | ||
return obj.cast<std::shared_ptr<VB>>(); | ||
} | ||
|
||
// There is no type_caster<std::weak_ptr<VB>>, and to minimize code complexity | ||
// we do not want to add one, therefore we have to return a shared_ptr here. | ||
template <typename VB> | ||
std::shared_ptr<VB> rtrn_potentially_slicing_shared_ptr(py::handle obj) { | ||
return py::potentially_slicing_weak_ptr<VB>(obj).lock(); | ||
} | ||
|
||
template <typename VB> | ||
struct SpOwner { | ||
void set_sp(const std::shared_ptr<VB> &sp_) { sp = sp_; } | ||
|
||
int get_code() const { | ||
if (!sp) { | ||
return -888; | ||
} | ||
return sp->get_code(); | ||
} | ||
|
||
const char *get_trampoline_state() const { return determine_trampoline_state(sp); } | ||
|
||
private: | ||
std::shared_ptr<VB> sp; | ||
}; | ||
|
||
template <typename VB> | ||
struct WpOwner { | ||
void set_wp(const std::weak_ptr<VB> &wp_) { wp = wp_; } | ||
|
||
int get_code() const { | ||
auto sp = wp.lock(); | ||
if (!sp) { | ||
return -999; | ||
} | ||
return sp->get_code(); | ||
} | ||
|
||
const char *get_trampoline_state() const { return determine_trampoline_state(wp.lock()); } | ||
|
||
private: | ||
std::weak_ptr<VB> wp; | ||
}; | ||
|
||
template <typename VB> | ||
void wrap(py::module_ &m, | ||
const char *roc_pyname, | ||
const char *rps_pyname, | ||
const char *spo_pyname, | ||
const char *wpo_pyname) { | ||
m.def(roc_pyname, rtrn_obj_cast_shared_ptr<VB>); | ||
m.def(rps_pyname, rtrn_potentially_slicing_shared_ptr<VB>); | ||
|
||
py::classh<SpOwner<VB>>(m, spo_pyname) | ||
.def(py::init<>()) | ||
.def("set_sp", &SpOwner<VB>::set_sp) | ||
.def("get_code", &SpOwner<VB>::get_code) | ||
.def("get_trampoline_state", &SpOwner<VB>::get_trampoline_state); | ||
|
||
py::classh<WpOwner<VB>>(m, wpo_pyname) | ||
.def(py::init<>()) | ||
.def("set_wp", | ||
[](WpOwner<VB> &self, py::handle obj) { | ||
self.set_wp(obj.cast<std::shared_ptr<VB>>()); | ||
}) | ||
.def("set_wp_potentially_slicing", | ||
[](WpOwner<VB> &self, py::handle obj) { | ||
self.set_wp(py::potentially_slicing_weak_ptr<VB>(obj)); | ||
}) | ||
.def("get_code", &WpOwner<VB>::get_code) | ||
.def("get_trampoline_state", &WpOwner<VB>::get_trampoline_state); | ||
} | ||
|
||
} // namespace potentially_slicing_weak_ptr | ||
} // namespace pybind11_tests | ||
|
||
using namespace pybind11_tests::potentially_slicing_weak_ptr; | ||
|
||
TEST_SUBMODULE(potentially_slicing_weak_ptr, m) { | ||
py::classh<VirtBaseSH, PyVirtBaseSH>(m, "VirtBaseSH") | ||
.def(py::init<>()) | ||
.def("get_code", &VirtBaseSH::get_code); | ||
|
||
py::class_<VirtBaseSP, std::shared_ptr<VirtBaseSP>, PyVirtBaseSP>(m, "VirtBaseSP") | ||
.def(py::init<>()) | ||
.def("get_code", &VirtBaseSP::get_code); | ||
|
||
wrap<VirtBaseSH>(m, | ||
"SH_rtrn_obj_cast_shared_ptr", | ||
"SH_rtrn_potentially_slicing_shared_ptr", | ||
"SH_SpOwner", | ||
"SH_WpOwner"); | ||
|
||
wrap<VirtBaseSP>(m, | ||
"SP_rtrn_obj_cast_shared_ptr", | ||
"SP_rtrn_potentially_slicing_shared_ptr", | ||
"SP_SpOwner", | ||
"SP_WpOwner"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] A short inline comment explaining why the call to load_as_shared_ptr uses force_potentially_slicing_shared_ptr=true would help maintain clarity for future readers.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
// Reusing shared_ptr code to minimize code complexity.