Replace all SMART_HOLDER_WIP comments with reminders, notes, or pointers. (#5336)

The SMART_HOLDER_WIP comments are mostly from 2021. In the meantime, the
smart_holder code was extremely thoroughly tested in the Google codebase
(production code). Additionally, testing via PyCLIF-pybind11 provided
thousands of diverse use cases that needed to satisfy many million unit tests
(the success rate was about 99.999%).
This commit is contained in:
Ralf W. Grosse-Kunstleve 2024-08-27 00:23:51 +07:00 committed by GitHub
parent 04d9f84f26
commit 5ed381daac
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 16 additions and 15 deletions

View File

@ -215,7 +215,7 @@ struct smart_holder {
// race conditions, but in the context of Python it is a bug (elsewhere) // race conditions, but in the context of Python it is a bug (elsewhere)
// if the Global Interpreter Lock (GIL) is not being held when this code // if the Global Interpreter Lock (GIL) is not being held when this code
// is reached. // is reached.
// SMART_HOLDER_WIP: IMPROVABLE: assert(GIL is held). // PYBIND11:REMINDER: This may need to be protected by a mutex in free-threaded Python.
if (vptr.use_count() != 1) { if (vptr.use_count() != 1) {
throw std::invalid_argument(std::string("Cannot disown use_count != 1 (") + context throw std::invalid_argument(std::string("Cannot disown use_count != 1 (") + context
+ ")."); + ").");
@ -277,29 +277,32 @@ struct smart_holder {
return hld; return hld;
} }
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). // Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void disown() { void disown() {
reset_vptr_deleter_armed_flag(false); reset_vptr_deleter_armed_flag(false);
is_disowned = true; is_disowned = true;
} }
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). // Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void reclaim_disowned() { void reclaim_disowned() {
reset_vptr_deleter_armed_flag(true); reset_vptr_deleter_armed_flag(true);
is_disowned = false; is_disowned = false;
} }
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). // Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void release_disowned() { vptr.reset(); } void release_disowned() { vptr.reset(); }
// SMART_HOLDER_WIP: review this function.
void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") const { void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") const {
ensure_is_not_disowned(context); ensure_is_not_disowned(context);
ensure_vptr_is_using_builtin_delete(context); ensure_vptr_is_using_builtin_delete(context);
ensure_use_count_1(context); ensure_use_count_1(context);
} }
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). // Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void release_ownership() { void release_ownership() {
reset_vptr_deleter_armed_flag(false); reset_vptr_deleter_armed_flag(false);
release_disowned(); release_disowned();

View File

@ -475,7 +475,7 @@ inline PyObject *make_new_instance(PyTypeObject *type);
#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT #ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code. // PYBIND11:REMINDER: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo); inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
PYBIND11_NAMESPACE_BEGIN(smart_holder_type_caster_support) PYBIND11_NAMESPACE_BEGIN(smart_holder_type_caster_support)
@ -568,8 +568,7 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&src,
if (static_cast<void *>(src.get()) == src_raw_void_ptr) { if (static_cast<void *>(src.get()) == src_raw_void_ptr) {
// This is a multiple-inheritance situation that is incompatible with the current // This is a multiple-inheritance situation that is incompatible with the current
// shared_from_this handling (see PR #3023). // shared_from_this handling (see PR #3023). Is there a better solution?
// SMART_HOLDER_WIP: IMPROVABLE: Is there a better solution?
src_raw_void_ptr = nullptr; src_raw_void_ptr = nullptr;
} }
auto smhldr = smart_holder::from_unique_ptr(std::move(src), src_raw_void_ptr); auto smhldr = smart_holder::from_unique_ptr(std::move(src), src_raw_void_ptr);
@ -623,8 +622,8 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr<T> &src,
void *src_raw_void_ptr = static_cast<void *>(src_raw_ptr); void *src_raw_void_ptr = static_cast<void *>(src_raw_ptr);
const detail::type_info *tinfo = st.second; const detail::type_info *tinfo = st.second;
if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) {
// SMART_HOLDER_WIP: MISSING: Enforcement of consistency with existing smart_holder. // PYBIND11:REMINDER: MISSING: Enforcement of consistency with existing smart_holder.
// SMART_HOLDER_WIP: MISSING: keep_alive. // PYBIND11:REMINDER: MISSING: keep_alive.
return existing_inst; return existing_inst;
} }

View File

@ -15,14 +15,13 @@
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail) PYBIND11_NAMESPACE_BEGIN(detail)
// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code. // PYBIND11:REMINDER: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo); inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(detail)
// The original core idea for this struct goes back to PyCLIF: // The original core idea for this struct goes back to PyCLIF:
// https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37 // https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37
// URL provided here mainly to give proper credit. To fully explain the `HoldPyObj` feature, more // URL provided here mainly to give proper credit.
// context is needed (SMART_HOLDER_WIP).
struct trampoline_self_life_support { struct trampoline_self_life_support {
detail::value_and_holder v_h; detail::value_and_holder v_h;

View File

@ -62,7 +62,7 @@ def test_shared_ptr_arg_identity():
del obj del obj
pytest.gc_collect() pytest.gc_collect()
# SMART_HOLDER_WIP: the behavior below is DIFFERENT from PR #2839 # NOTE: the behavior below is DIFFERENT from PR #2839
# python reference is gone because it is not an Alias instance # python reference is gone because it is not an Alias instance
assert objref() is None assert objref() is None
assert tester.has_python_instance() is False assert tester.has_python_instance() is False