From 5ed381daac5035cad9745824f96d04e7a35924e7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 27 Aug 2024 00:23:51 +0700 Subject: [PATCH] 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%). --- include/pybind11/detail/struct_smart_holder.h | 15 +++++++++------ include/pybind11/detail/type_caster_base.h | 9 ++++----- include/pybind11/trampoline_self_life_support.h | 5 ++--- ...test_class_sh_trampoline_shared_ptr_cpp_arg.py | 2 +- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index b1e24d7bb3..980fc3699b 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -215,7 +215,7 @@ struct smart_holder { // 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 // 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) { throw std::invalid_argument(std::string("Cannot disown use_count != 1 (") + context + ")."); @@ -277,29 +277,32 @@ struct smart_holder { 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() { reset_vptr_deleter_armed_flag(false); 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() { reset_vptr_deleter_armed_flag(true); 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(); } - // SMART_HOLDER_WIP: review this function. void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") const { ensure_is_not_disowned(context); ensure_vptr_is_using_builtin_delete(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() { reset_vptr_deleter_armed_flag(false); release_disowned(); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index f1a5d0d8dc..7cdeabdccf 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -475,7 +475,7 @@ inline PyObject *make_new_instance(PyTypeObject *type); #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); PYBIND11_NAMESPACE_BEGIN(smart_holder_type_caster_support) @@ -568,8 +568,7 @@ handle smart_holder_from_unique_ptr(std::unique_ptr &&src, if (static_cast(src.get()) == src_raw_void_ptr) { // This is a multiple-inheritance situation that is incompatible with the current - // shared_from_this handling (see PR #3023). - // SMART_HOLDER_WIP: IMPROVABLE: Is there a better solution? + // shared_from_this handling (see PR #3023). Is there a better solution? src_raw_void_ptr = nullptr; } 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 &src, void *src_raw_void_ptr = static_cast(src_raw_ptr); const detail::type_info *tinfo = st.second; if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { - // SMART_HOLDER_WIP: MISSING: Enforcement of consistency with existing smart_holder. - // SMART_HOLDER_WIP: MISSING: keep_alive. + // PYBIND11:REMINDER: MISSING: Enforcement of consistency with existing smart_holder. + // PYBIND11:REMINDER: MISSING: keep_alive. return existing_inst; } diff --git a/include/pybind11/trampoline_self_life_support.h b/include/pybind11/trampoline_self_life_support.h index cef69632a7..a30ffda3b5 100644 --- a/include/pybind11/trampoline_self_life_support.h +++ b/include/pybind11/trampoline_self_life_support.h @@ -15,14 +15,13 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) 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); PYBIND11_NAMESPACE_END(detail) // The original core idea for this struct goes back to PyCLIF: // 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 -// context is needed (SMART_HOLDER_WIP). +// URL provided here mainly to give proper credit. struct trampoline_self_life_support { detail::value_and_holder v_h; diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py index 13daeee237..85977542e4 100644 --- a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py +++ b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py @@ -62,7 +62,7 @@ def test_shared_ptr_arg_identity(): del obj 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 assert objref() is None assert tester.has_python_instance() is False