Skip to content

Commit

Permalink
Suggestions from code review
Browse files Browse the repository at this point in the history
1) Renaming: is_recursive_container and
   MutuallyRecursiveContainerPair(MV|VM)
2) Avoid ambiguous specializations of is_recursive_container
  • Loading branch information
franzpoeschel committed Apr 24, 2023
1 parent 0ecf5f0 commit 9ba38bf
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 34 deletions.
42 changes: 25 additions & 17 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -832,30 +832,38 @@ struct is_move_constructible : std::is_move_constructible<T> {};

// True if Container has a dependent type mapped_type that is equivalent
// to Container itself
// Actual implementation in the SFINAE specializations below
// Actual implementation in the SFINAE specialization below
template <typename Container, typename MappedType = Container>
struct sfinae_is_container_with_self_referential_mapped_type : std::false_type {};
struct is_container_with_recursive_mapped_type : std::false_type {};

// Tie-breaking between the mapped_type and the value_type specializations is trivial:
// The specializations are only valid if both conditions are fulfilled:
// 1) The mapped_type (respectively value_type) exists
// This specialization is only valid if both conditions are fulfilled:
// 1) The mapped_type exists
// 2) And it is equivalent to Container
// So, in each case, only one specialization will activate.
template <typename Container>
struct sfinae_is_container_with_self_referential_mapped_type<Container,
typename Container::mapped_type>
struct is_container_with_recursive_mapped_type<Container, typename Container::mapped_type>
: std::true_type {};

// True if Container has a dependent type value_type that is equivalent
// to Container itself
// Actual implementation in the SFINAE specialization below
template <typename Container, typename MappedType = Container>
struct is_container_with_recursive_value_type : std::false_type {};

// This specialization is only valid if both conditions are fulfilled:
// 1) The value_type exists
// 2) And it is equivalent to Container
template <typename Container>
struct sfinae_is_container_with_self_referential_mapped_type<Container,
typename Container::value_type>
struct is_container_with_recursive_value_type<Container, typename Container::value_type>
: std::true_type {};

// Use a helper struct in order to give this a nicer public API without helper template parameter.
// This makes this struct nicer to specialize by users.
template <typename Container>
struct is_container_with_self_referential_mapped_type
: sfinae_is_container_with_self_referential_mapped_type<Container> {};
// True constant if the type contains itself recursively.
// By default, this will check the mapped_type and value_type dependent types.
// In more complex recursion patterns, users can specialize this struct.
// The second template parameter SFINAE=void is for use of std::enable_if in specializations.
// An example is found in tests/test_stl_binders.cpp.
template <typename Container, typename SFINAE = void>
struct is_recursive_container : any_of<is_container_with_recursive_value_type<Container>,
is_container_with_recursive_mapped_type<Container>> {};

// Specialization for types that appear to be copy constructible but also look like stl containers
// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if
Expand All @@ -867,7 +875,7 @@ struct is_copy_constructible<
all_of<std::is_copy_constructible<Container>,
std::is_same<typename Container::value_type &, typename Container::reference>,
// Avoid infinite recursion
negation<is_container_with_self_referential_mapped_type<Container>>>::value>>
negation<is_recursive_container<Container>>>::value>>
: is_copy_constructible<typename Container::value_type> {};

// Likewise for std::pair
Expand All @@ -888,7 +896,7 @@ struct is_copy_assignable<
all_of<std::is_copy_assignable<Container>,
std::is_same<typename Container::value_type &, typename Container::reference>,
// Avoid infinite recursion
negation<is_container_with_self_referential_mapped_type<Container>>>::value>>
negation<is_recursive_container<Container>>>::value>>
: is_copy_assignable<typename Container::value_type> {};
template <typename T1, typename T2>
struct is_copy_assignable<std::pair<T1, T2>>
Expand Down
9 changes: 4 additions & 5 deletions include/pybind11/stl_bind.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,10 @@ struct is_comparable<
/* For a vector/map data structure, recursively check the value type
(which is std::pair for maps) */
template <typename T>
struct is_comparable<
T,
enable_if_t<container_traits<T>::is_vector &&
// Avoid this instantiation if the type is recursive
negation<is_container_with_self_referential_mapped_type<T>>::value>> {
struct is_comparable<T,
enable_if_t<container_traits<T>::is_vector &&
// Avoid this instantiation if the type is recursive
negation<is_recursive_container<T>>::value>> {
static constexpr const bool value = is_comparable<typename T::value_type>::value;
};

Expand Down
22 changes: 10 additions & 12 deletions tests/test_stl_binders.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,18 @@ struct RecursiveMap : std::map<int, RecursiveMap> {
* pybind11::detail::is_container_with_self_referential_mapped_type, thus
* manually telling pybind11 about the recursion.
*/
struct MutuallyRecursiveContainerPairA;
struct MutuallyRecursiveContainerPairB;
struct MutuallyRecursiveContainerPairMV;
struct MutuallyRecursiveContainerPairVM;

struct MutuallyRecursiveContainerPairA : std::map<int, MutuallyRecursiveContainerPairB> {};
struct MutuallyRecursiveContainerPairB : std::vector<MutuallyRecursiveContainerPairA> {};
struct MutuallyRecursiveContainerPairMV : std::map<int, MutuallyRecursiveContainerPairVM> {};
struct MutuallyRecursiveContainerPairVM : std::vector<MutuallyRecursiveContainerPairMV> {};

namespace pybind11 {
namespace detail {
template <>
struct is_container_with_self_referential_mapped_type<MutuallyRecursiveContainerPairB>
: std::true_type {};
template <>
struct is_container_with_self_referential_mapped_type<MutuallyRecursiveContainerPairA>
: std::true_type {};
template <typename SFINAE>
struct is_recursive_container<MutuallyRecursiveContainerPairMV, SFINAE> : std::true_type {};
template <typename SFINAE>
struct is_recursive_container<MutuallyRecursiveContainerPairVM, SFINAE> : std::true_type {};
} // namespace detail
} // namespace pybind11

Expand Down Expand Up @@ -169,8 +167,8 @@ TEST_SUBMODULE(stl_binders, m) {
// Bind recursive container types
py::bind_vector<RecursiveVector>(m, "RecursiveVector");
py::bind_map<RecursiveMap>(m, "RecursiveMap");
py::bind_map<MutuallyRecursiveContainerPairA>(m, "MutuallyRecursiveContainerPairA");
py::bind_vector<MutuallyRecursiveContainerPairB>(m, "MutuallyRecursiveContainerPairB");
py::bind_map<MutuallyRecursiveContainerPairMV>(m, "MutuallyRecursiveContainerPairMV");
py::bind_vector<MutuallyRecursiveContainerPairVM>(m, "MutuallyRecursiveContainerPairVM");

// The rest depends on numpy:
try {
Expand Down

0 comments on commit 9ba38bf

Please sign in to comment.