From 1ae7d52c0ea63634d2949ba2935414be46793a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Gs=C3=A4nger?= Date: Fri, 16 Aug 2019 11:26:42 +0200 Subject: [PATCH 1/3] test pair-copyability on C++17 upwards The stdlib falsely detects containers like M=std::map as copyable, even when one of T and U is not copyable. Therefore we cannot rely on the stdlib dismissing std::pair by itself, even on C++17. --- include/pybind11/cast.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e1bbf03aa1..8cd04e676e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -797,12 +797,11 @@ template struct is_copy_constructible> >::value>> : is_copy_constructible {}; -#if !defined(PYBIND11_CPP17) -// Likewise for std::pair before C++17 (which mandates that the copy constructor not exist when the -// two types aren't themselves copy constructible). +// Likewise for std::pair +// (after C++17 it is mandatory that the copy constructor not exist when the two types aren't themselves +// copy constructible, but this can not be relied upon when T1 or T2 are themselves containers). template struct is_copy_constructible> : all_of, is_copy_constructible> {}; -#endif NAMESPACE_END(detail) From 46d0c5e4d860e2279bbebcce7a6b9dd67e14e936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Gs=C3=A4nger?= Date: Fri, 16 Aug 2019 18:53:05 +0200 Subject: [PATCH 2/3] fix is_copy_assignable bind_map used std::is_copy_assignable which suffers from the same problems as std::is_copy_constructible, therefore the same fix has been applied. --- include/pybind11/cast.h | 9 +++++++++ include/pybind11/stl_bind.h | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8cd04e676e..90407eb984 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -803,6 +803,15 @@ template struct is_copy_constructible struct is_copy_constructible> : all_of, is_copy_constructible> {}; +// The same problems arise with std::is_copy_assignable, so we use the same workaround. +template struct is_copy_assignable : std::is_copy_assignable {}; +template struct is_copy_assignable, + std::is_same + >::value>> : is_copy_assignable {}; +template struct is_copy_assignable> + : all_of, is_copy_assignable> {}; + NAMESPACE_END(detail) // polymorphic_type_hook::get(src, tinfo) determines whether the object pointed diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index d3adaed3a2..62bd908196 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -512,7 +512,7 @@ template void map_assignment(const Args & // Map assignment when copy-assignable: just copy the value template -void map_assignment(enable_if_t::value, Class_> &cl) { +void map_assignment(enable_if_t::value, Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; @@ -528,7 +528,7 @@ void map_assignment(enable_if_t void map_assignment(enable_if_t< - !std::is_copy_assignable::value && + !is_copy_assignable::value && is_copy_constructible::value, Class_> &cl) { using KeyType = typename Map::key_type; From 3600093285dc5b8de9b98cb2e0375854875241b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Gs=C3=A4nger?= Date: Fri, 16 Aug 2019 18:54:07 +0200 Subject: [PATCH 3/3] created tests for copyability --- tests/test_stl_binders.cpp | 22 ++++++++++++++++++++++ tests/test_stl_binders.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index a88b589e13..8688874091 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -54,6 +54,14 @@ template Map *times_ten(int n) { return m; } +template NestMap *times_hundred(int n) { + auto m = new NestMap(); + for (int i = 1; i <= n; i++) + for (int j = 1; j <= n; j++) + (*m)[i].emplace(int(j*10), E_nc(100*j)); + return m; +} + TEST_SUBMODULE(stl_binders, m) { // test_vector_int py::bind_vector>(m, "VectorInt", py::buffer_protocol()); @@ -85,6 +93,20 @@ TEST_SUBMODULE(stl_binders, m) { m.def("get_mnc", ×_ten>, py::return_value_policy::reference); py::bind_map>(m, "UmapENC"); m.def("get_umnc", ×_ten>, py::return_value_policy::reference); + // Issue #1885: binding nested std::map> with E non-copyable + py::bind_map>>(m, "MapVecENC"); + m.def("get_nvnc", [](int n) + { + auto m = new std::map>(); + for (int i = 1; i <= n; i++) + for (int j = 1; j <= n; j++) + (*m)[i].emplace_back(j); + return m; + }, py::return_value_policy::reference); + py::bind_map>>(m, "MapMapENC"); + m.def("get_nmnc", ×_hundred>>, py::return_value_policy::reference); + py::bind_map>>(m, "UmapUmapENC"); + m.def("get_numnc", ×_hundred>>, py::return_value_policy::reference); // test_vector_buffer py::bind_vector>(m, "VectorUChar", py::buffer_protocol()); diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 6d5a159833..b83a587f26 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -212,6 +212,44 @@ def test_noncopyable_containers(): assert vsum == 150 + # nested std::map + nvnc = m.get_nvnc(5) + for i in range(1, 6): + for j in range(0, 5): + assert nvnc[i][j].value == j + 1 + + for k, v in nvnc.items(): + for i, j in enumerate(v, start=1): + assert j.value == i + + # nested std::map + nmnc = m.get_nmnc(5) + for i in range(1, 6): + for j in range(10, 60, 10): + assert nmnc[i][j].value == 10 * j + + vsum = 0 + for k_o, v_o in nmnc.items(): + for k_i, v_i in v_o.items(): + assert v_i.value == 10 * k_i + vsum += v_i.value + + assert vsum == 7500 + + # nested std::unordered_map + numnc = m.get_numnc(5) + for i in range(1, 6): + for j in range(10, 60, 10): + assert numnc[i][j].value == 10 * j + + vsum = 0 + for k_o, v_o in numnc.items(): + for k_i, v_i in v_o.items(): + assert v_i.value == 10 * k_i + vsum += v_i.value + + assert vsum == 7500 + def test_map_delitem(): mm = m.MapStringDouble()