Skip to content

[smart_holder] Replace all SMART_HOLDER_WIP comments with reminders, notes, or pointers. #5336

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

Merged
merged 1 commit into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 9 additions & 6 deletions include/pybind11/detail/struct_smart_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
+ ").");
Expand Down Expand Up @@ -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();
Expand Down
9 changes: 4 additions & 5 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
// 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);
Expand Down Expand Up @@ -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);
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;
}

Expand Down
5 changes: 2 additions & 3 deletions include/pybind11/trampoline_self_life_support.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down