Skip to content

Commit

Permalink
[smart_holder] Simplification: Enable smart_holder functionality unco…
Browse files Browse the repository at this point in the history
…nditionally. (#5531)

* git merge --squash purge_internals_versions_4_5

* Remove PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT, set PYBIND11_INTERNALS_VERSION 7

* Remove all uses of PYBIND11_SMART_HOLDER_ENABLED under include/pybind11

* Remove obsolete PYBIND11_ACTUALLY_USING_SMART_HOLDER_AS_DEFAULT macro.

* Remove PYBIND11_SMART_HOLDER_ENABLED in ubench/holder_comparison.cpp

* Remove all uses of PYBIND11_SMART_HOLDER_ENABLED under tests/

* Remove `#define PYBIND11_SMART_HOLDER_ENABLED`

* Remove all uses of PYBIND11_SMART_HOLDER_TYPE_CASTERS under tests/

* Remove all uses of PYBIND11_TYPE_CASTER_BASE_HOLDER under tests/

* Add missing `#include <cstdint>`

Example error message (🐍 3.11 • ubuntu-latest • x64, GNU 13.3.0):

```
include/pybind11/detail/value_and_holder.h:56:52: error: ‘uint8_t’ is not a member of ‘std’; did you mean ‘wint_t’?
   56 |             inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed;
      |                                                    ^~~~~~~
```

* Change PYBIND11_INTERNALS_VERSION to 106: It will be changed to 7 in a follow-on PR that actually changes the internals.
  • Loading branch information
rwgk authored Feb 22, 2025
1 parent dc37cba commit 5ab036b
Show file tree
Hide file tree
Showing 48 changed files with 35 additions and 440 deletions.
14 changes: 0 additions & 14 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,20 +102,6 @@ jobs:
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="/DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT /GR /EHsc"
# Exercise PYBIND11_SMART_HOLDER_DISABLE
# with recent (or ideally latest) released Python version.
- runs-on: ubuntu-latest
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_SMART_HOLDER_DISABLE"
- runs-on: macos-13
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_SMART_HOLDER_DISABLE"
- runs-on: windows-2022
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="/DPYBIND11_SMART_HOLDER_DISABLE /GR /EHsc"
exclude:
# The setup-python action currently doesn't have graalpy for windows
- python: 'graalpy-24.1'
Expand Down
2 changes: 0 additions & 2 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,7 @@ struct type_record {
/// Solves pybind/pybind11#1446
bool release_gil_before_calling_cpp_dtor : 1;

#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
holder_enum_t holder_enum_v = holder_enum_t::undefined;
#endif

PYBIND11_NOINLINE void add_base(const std::type_info &base, void *(*caster)(void *) ) {
auto *base_info = detail::get_type_info(base, false);
Expand Down
16 changes: 3 additions & 13 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -836,8 +836,6 @@ struct copyable_holder_caster : public type_caster_base<type> {
holder_type holder;
};

#ifdef PYBIND11_SMART_HOLDER_ENABLED

template <typename, typename SFINAE = void>
struct copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled : std::true_type {};

Expand Down Expand Up @@ -928,13 +926,13 @@ struct copyable_holder_caster<
return;
}
throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
# if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for "
"type information)");
# else
#else
"of type '"
+ type_id<std::shared_ptr<type>>() + "''");
# endif
#endif
}

template <typename T = std::shared_ptr<type>,
Expand Down Expand Up @@ -968,8 +966,6 @@ struct copyable_holder_caster<
std::shared_ptr<type> shared_ptr_storage;
};

#endif // PYBIND11_SMART_HOLDER_ENABLED

/// Specialize for the common std::shared_ptr, so users don't need to
template <typename T>
class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> {};
Expand All @@ -990,8 +986,6 @@ struct move_only_holder_caster {
static constexpr auto name = type_caster_base<type>::name;
};

#ifdef PYBIND11_SMART_HOLDER_ENABLED

template <typename, typename SFINAE = void>
struct move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled : std::true_type {};

Expand Down Expand Up @@ -1129,8 +1123,6 @@ struct move_only_holder_caster<
std::shared_ptr<std::unique_ptr<type, deleter>> unique_ptr_storage;
};

#endif // PYBIND11_SMART_HOLDER_ENABLED

template <typename type, typename deleter>
class type_caster<std::unique_ptr<type, deleter>>
: public move_only_holder_caster<type, std::unique_ptr<type, deleter>> {};
Expand Down Expand Up @@ -1169,10 +1161,8 @@ struct is_holder_type
template <typename base, typename deleter>
struct is_holder_type<base, std::unique_ptr<base, deleter>> : std::true_type {};

#ifdef PYBIND11_SMART_HOLDER_ENABLED
template <typename base>
struct is_holder_type<base, smart_holder> : std::true_type {};
#endif

#ifdef PYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION // See PR #4888

Expand Down
3 changes: 0 additions & 3 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -605,11 +605,8 @@ struct instance {
bool simple_instance_registered : 1;
/// If true, get_internals().patients has an entry for this object
bool has_patients : 1;
// Cannot use PYBIND11_INTERNALS_VERSION >= 106 here without refactoring.
#if PYBIND11_VERSION_MAJOR >= 3
/// If true, this Python object needs to be kept alive for the lifetime of the C++ value.
bool is_alias : 1;
#endif

/// Initializes all of the above type/values/holders data (but not the instance values
/// themselves)
Expand Down
4 changes: 0 additions & 4 deletions include/pybind11/detail/init.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,6 @@ void construct(value_and_holder &v_h, Alias<Class> &&result, bool) {
v_h.value_ptr() = new Alias<Class>(std::move(result));
}

#ifdef PYBIND11_SMART_HOLDER_ENABLED

template <typename T, typename D>
smart_holder init_smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr,
bool void_cast_raw_ptr) {
Expand Down Expand Up @@ -268,8 +266,6 @@ void construct(value_and_holder &v_h,
v_h.type->init_instance(v_h.inst, &smhldr);
}

#endif // PYBIND11_SMART_HOLDER_ENABLED

// Implementing class for py::init<...>()
template <typename... Args>
struct constructor {
Expand Down
23 changes: 3 additions & 20 deletions include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,11 @@
/// further ABI-incompatible changes may be made before the ABI is officially
/// changed to the new version.
#ifndef PYBIND11_INTERNALS_VERSION
# if PYBIND11_VERSION_MAJOR >= 3
# define PYBIND11_INTERNALS_VERSION 106
# else
# define PYBIND11_INTERNALS_VERSION 6
# endif
# define PYBIND11_INTERNALS_VERSION 106
#endif

#if PYBIND11_INTERNALS_VERSION < 6
# error "PYBIND11_INTERNALS_VERSION 6 is the minimum for all platforms for pybind11v3."
#if PYBIND11_INTERNALS_VERSION < 106
# error "PYBIND11_INTERNALS_VERSION 106 is the minimum (SPECIAL SITUATION)."
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down Expand Up @@ -215,10 +211,6 @@ struct internals {
}
};

#if PYBIND11_INTERNALS_VERSION >= 106

# define PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT

enum class holder_enum_t : uint8_t {
undefined,
std_unique_ptr, // Default, lacking interop with std::shared_ptr.
Expand All @@ -227,13 +219,6 @@ enum class holder_enum_t : uint8_t {
custom_holder,
};

#endif

#if defined(PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT) \
&& !defined(PYBIND11_SMART_HOLDER_DISABLE)
# define PYBIND11_SMART_HOLDER_ENABLED
#endif

/// Additional type information which does not fit into the PyTypeObject.
/// Changes to this struct also require bumping `PYBIND11_INTERNALS_VERSION`.
struct type_info {
Expand Down Expand Up @@ -262,9 +247,7 @@ struct type_info {
bool default_holder : 1;
/* true if this is a type registered with py::module_local */
bool module_local : 1;
#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
holder_enum_t holder_enum_v = holder_enum_t::undefined;
#endif
};

#define PYBIND11_INTERNALS_ID \
Expand Down
6 changes: 0 additions & 6 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -511,8 +511,6 @@ inline PyThreadState *get_thread_state_unchecked() {
void keep_alive_impl(handle nurse, handle patient);
inline PyObject *make_new_instance(PyTypeObject *type);

#ifdef PYBIND11_SMART_HOLDER_ENABLED

// PYBIND11:REMINDER: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);

Expand Down Expand Up @@ -868,8 +866,6 @@ struct load_helper : value_and_holder_helper {

PYBIND11_NAMESPACE_END(smart_holder_type_caster_support)

#endif // PYBIND11_SMART_HOLDER_ENABLED

class type_caster_generic {
public:
PYBIND11_NOINLINE explicit type_caster_generic(const std::type_info &type_info)
Expand Down Expand Up @@ -974,7 +970,6 @@ class type_caster_generic {

// Base methods for generic caster; there are overridden in copyable_holder_caster
void load_value(value_and_holder &&v_h) {
#ifdef PYBIND11_SMART_HOLDER_ENABLED
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
smart_holder_type_caster_support::value_and_holder_helper v_h_helper;
v_h_helper.loaded_v_h = v_h;
Expand All @@ -984,7 +979,6 @@ class type_caster_generic {
return;
}
}
#endif
auto *&vptr = v_h.value_ptr();
// Lazy allocation for unallocated values:
if (vptr == nullptr) {
Expand Down
13 changes: 1 addition & 12 deletions include/pybind11/detail/using_smart_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,18 @@
#pragma once

#include "common.h"
#include "internals.h"
#include "struct_smart_holder.h"

#include <type_traits>

#ifdef PYBIND11_SMART_HOLDER_ENABLED
# include "struct_smart_holder.h"
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

#ifdef PYBIND11_SMART_HOLDER_ENABLED
using pybindit::memory::smart_holder;
#endif

PYBIND11_NAMESPACE_BEGIN(detail)

#ifdef PYBIND11_SMART_HOLDER_ENABLED
template <typename H>
using is_smart_holder = std::is_same<H, smart_holder>;
#else
template <typename>
struct is_smart_holder : std::false_type {};
#endif

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
1 change: 1 addition & 0 deletions include/pybind11/detail/value_and_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "common.h"

#include <cstddef>
#include <cstdint>
#include <typeinfo>

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down
19 changes: 1 addition & 18 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1448,9 +1448,7 @@ class generic_type : public object {
tinfo->simple_ancestors = true;
tinfo->default_holder = rec.default_holder;
tinfo->module_local = rec.module_local;
#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
tinfo->holder_enum_v = rec.holder_enum_v;
#endif

with_internals([&](internals &internals) {
auto tindex = std::type_index(*rec.type);
Expand Down Expand Up @@ -1665,8 +1663,6 @@ PYBIND11_NAMESPACE_END(detail)
template <typename T, typename D, typename SFINAE = void>
struct property_cpp_function : detail::property_cpp_function_classic<T, D> {};

#ifdef PYBIND11_SMART_HOLDER_ENABLED

PYBIND11_NAMESPACE_BEGIN(detail)

template <typename T, typename D, typename SFINAE = void>
Expand Down Expand Up @@ -1844,17 +1840,14 @@ struct property_cpp_function<
detail::both_t_and_d_use_type_caster_base<T, typename D::element_type>>::value>>
: detail::property_cpp_function_sh_unique_ptr_member<T, D> {};

#endif // PYBIND11_SMART_HOLDER_ENABLED

#if defined(PYBIND11_USE_SMART_HOLDER_AS_DEFAULT) && defined(PYBIND11_SMART_HOLDER_ENABLED)
#if defined(PYBIND11_USE_SMART_HOLDER_AS_DEFAULT)
// NOTE: THIS IS MEANT FOR STRESS-TESTING ONLY!
// As of PR #5257, for production use, there is no longer a strong reason to make
// smart_holder the default holder:
// Simply use `py::classh` (see below) instead of `py::class_` as needed.
// Running the pybind11 unit tests with smart_holder as the default holder is to ensure
// that `py::smart_holder` / `py::classh` is backward-compatible with all pre-existing
// functionality.
# define PYBIND11_ACTUALLY_USING_SMART_HOLDER_AS_DEFAULT
template <typename>
using default_holder_type = smart_holder;
#else
Expand Down Expand Up @@ -1913,7 +1906,6 @@ class class_ : public detail::generic_type {
// A more fitting name would be uses_unique_ptr_holder.
record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;

#ifdef PYBIND11_SMART_HOLDER_ENABLED
if (detail::is_instantiation<std::unique_ptr, holder_type>::value) {
record.holder_enum_v = detail::holder_enum_t::std_unique_ptr;
} else if (detail::is_instantiation<std::shared_ptr, holder_type>::value) {
Expand All @@ -1923,7 +1915,6 @@ class class_ : public detail::generic_type {
} else {
record.holder_enum_v = detail::holder_enum_t::custom_holder;
}
#endif

set_operator_new<type>(&record);

Expand Down Expand Up @@ -2265,8 +2256,6 @@ class class_ : public detail::generic_type {
init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
}

#ifdef PYBIND11_SMART_HOLDER_ENABLED

template <typename WrappedType>
static bool try_initialization_using_shared_from_this(holder_type *, WrappedType *, ...) {
return false;
Expand Down Expand Up @@ -2326,8 +2315,6 @@ class class_ : public detail::generic_type {
v_h.set_holder_constructed();
}

#endif // PYBIND11_SMART_HOLDER_ENABLED

// Deallocates an instance; via holder, if constructed; otherwise via operator delete.
// NOTE: The Python error indicator needs to cleared BEFORE this function is called.
// This is because we could be deallocating while cleaning up after a Python exception.
Expand Down Expand Up @@ -2393,8 +2380,6 @@ class class_ : public detail::generic_type {
}
};

#ifdef PYBIND11_SMART_HOLDER_ENABLED

// Supports easier switching between py::class_<T> and py::class_<T, py::smart_holder>:
// users can simply replace the `_` in `class_` with `h` or vice versa.
template <typename type_, typename... options>
Expand All @@ -2403,8 +2388,6 @@ class classh : public class_<type_, smart_holder, options...> {
using class_<type_, smart_holder, options...>::class_;
};

#endif

/// Binds an existing constructor taking arguments Args...
template <typename... Args>
detail::initimpl::constructor<Args...> init() {
Expand Down
12 changes: 3 additions & 9 deletions include/pybind11/trampoline_self_life_support.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,9 @@

#pragma once

#include "detail/internals.h"

#ifdef PYBIND11_SMART_HOLDER_ENABLED

# include "detail/common.h"
# include "detail/using_smart_holder.h"
# include "detail/value_and_holder.h"
#include "detail/common.h"
#include "detail/using_smart_holder.h"
#include "detail/value_and_holder.h"

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

Expand Down Expand Up @@ -62,5 +58,3 @@ struct trampoline_self_life_support {
};

PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

#endif // PYBIND11_SMART_HOLDER_ENABLED
6 changes: 1 addition & 5 deletions tests/test_class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,7 @@ TEST_SUBMODULE(class_, m) {
struct ToBeHeldByUniquePtr {};
py::class_<ToBeHeldByUniquePtr, std::unique_ptr<ToBeHeldByUniquePtr>>(m, "ToBeHeldByUniquePtr")
.def(py::init<>());
#ifdef PYBIND11_SMART_HOLDER_ENABLED
m.def("pass_unique_ptr", [](std::unique_ptr<ToBeHeldByUniquePtr> &&) {});
#else
m.attr("pass_unique_ptr") = py::none();
#endif

// test_inheritance
class Pet {
Expand Down Expand Up @@ -636,7 +632,7 @@ CHECK_NOALIAS(8);
CHECK_HOLDER(1, unique);
CHECK_HOLDER(2, unique);
CHECK_HOLDER(3, unique);
#ifndef PYBIND11_ACTUALLY_USING_SMART_HOLDER_AS_DEFAULT
#ifndef PYBIND11_USE_SMART_HOLDER_AS_DEFAULT
CHECK_HOLDER(4, unique);
CHECK_HOLDER(5, unique);
#endif
Expand Down
2 changes: 0 additions & 2 deletions tests/test_class.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ def test_instance_new():

def test_pass_unique_ptr():
obj = m.ToBeHeldByUniquePtr()
if m.pass_unique_ptr is None:
pytest.skip("smart_holder not available.")
with pytest.raises(RuntimeError) as execinfo:
m.pass_unique_ptr(obj)
assert str(execinfo.value).startswith(
Expand Down
Loading

0 comments on commit 5ab036b

Please sign in to comment.