Skip to content
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.

Sign up for GitHub

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

Factoring out make_constructor from type_caster_base (for reuse under PR #2672). #2798

Closed
wants to merge 2 commits into from
Closed

Factoring out make_constructor from type_caster_base (for reuse under PR #2672). #2798

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e3a4cb
Factoring out make_constructor from type_caster_base (for reuse under…
Jan 16, 2021
7281e03
Explicit use of make_constructor functions, instead of using (multipl…
Jan 17, 2021
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
49 changes: 26 additions & 23 deletions include/pybind11/cast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,30 @@ template <typename Container> struct is_copy_assignable<Container, enable_if_t<a
template <typename T1, typename T2> struct is_copy_assignable<std::pair<T1, T2>>
: all_of<is_copy_assignable<T1>, is_copy_assignable<T2>> {};

// Helper for type_caster_base.
struct make_constructor {
using Constructor = void *(*)(const void *);

/* Only enabled when the types are {copy,move}-constructible *and* when the type
does not have a private operator new implementation. */
template <typename T, typename = enable_if_t<is_copy_constructible<T>::value>>
static auto make_copy_constructor(const T *x) -> decltype(new T(*x), Constructor{}) {
return [](const void *arg) -> void * {
return new T(*reinterpret_cast<const T *>(arg));
};
}

template <typename T, typename = enable_if_t<std::is_move_constructible<T>::value>>
static auto make_move_constructor(const T *x) -> decltype(new T(std::move(*const_cast<T *>(x))), Constructor{}) {
return [](const void *arg) -> void * {
return new T(std::move(*const_cast<T *>(reinterpret_cast<const T *>(arg))));
};
}

static Constructor make_copy_constructor(...) { return nullptr; }
static Constructor make_move_constructor(...) { return nullptr; }
};

PYBIND11_NAMESPACE_END(detail)

// polymorphic_type_hook<itype>::get(src, tinfo) determines whether the object pointed
Expand Down Expand Up @@ -858,7 +882,8 @@ struct polymorphic_type_hook : public polymorphic_type_hook_base<itype> {};
PYBIND11_NAMESPACE_BEGIN(detail)

/// Generic type caster for objects stored on the heap
template <typename type> class type_caster_base : public type_caster_generic {
template <typename type> class type_caster_base : public type_caster_generic,
protected make_constructor {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uuuu, multiple inheritance. I guess it should be fine (there's nothing virtual about these casters, right?), but there's really no other way to get this done? :-/

Nitpick: Let's name it make_constructor_base or make_constructor_mixin, maybe, to make clear it's a class, rather than a function. (I guess that's obvious when used as base type, but still, make_constructor sounds like a function to me.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uuuu, multiple inheritance. I guess it should be fine (there's nothing virtual about these casters, right?), but there's really no other way to get this done? :-/

Within pybind11, i see no reason to actually have this be a static member function? So just adding these as inline functions to detail might work as well and avoid MI.

But we might be breaking the semi-public interface of type_caster_base (since it's only protected).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, non-virtual, so multiple inheritance is straightforward/safe.
But let me try something else, a little less conservative ... hang on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe this is the most interface-preserving change you can make, if we assume users subclassing type_caster_base would depend on this protected member.

And duplicating

template <typename T>
static auto make_copy_constructor(const T *x) -> decltype( detail::make_copy_constructor(x)) {
    return detail::make_copy_constructor(x);
}

just to avoid a mostly-safe type of MI is also quite silly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to see what you still have in mind, though!
Otherwise, I'm fine with this, except for maybe the suggested name change, if you agree on that.

using itype = intrinsic_t<type>;

public:
Expand Down Expand Up @@ -919,28 +944,6 @@ template <typename type> class type_caster_base : public type_caster_generic {

operator itype*() { return (type *) value; }
operator itype&() { if (!value) throw reference_cast_error(); return *((itype *) value); }

protected:
using Constructor = void *(*)(const void *);

/* Only enabled when the types are {copy,move}-constructible *and* when the type
does not have a private operator new implementation. */
template <typename T, typename = enable_if_t<is_copy_constructible<T>::value>>
static auto make_copy_constructor(const T *x) -> decltype(new T(*x), Constructor{}) {
return [](const void *arg) -> void * {
return new T(*reinterpret_cast<const T *>(arg));
};
}

template <typename T, typename = enable_if_t<std::is_move_constructible<T>::value>>
static auto make_move_constructor(const T *x) -> decltype(new T(std::move(*const_cast<T *>(x))), Constructor{}) {
return [](const void *arg) -> void * {
return new T(std::move(*const_cast<T *>(reinterpret_cast<const T *>(arg))));
};
}

static Constructor make_copy_constructor(...) { return nullptr; }
static Constructor make_move_constructor(...) { return nullptr; }
};

template <typename type, typename SFINAE = void> class type_caster : public type_caster_base<type> { };
Expand Down