From db09d9431989d8a43ce7473f57b0199ba221fbd8 Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 01:40:48 +0200 Subject: [PATCH 01/20] Create templated abstract classes KeysView, ValuesView and ItemsView, and implement them on-the-fly when wrapping any specific map type --- include/pybind11/stl_bind.h | 145 ++++++++++++++++++++++++------------ 1 file changed, 97 insertions(+), 48 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 22a29b476f..ac90c895be 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -10,10 +10,13 @@ #pragma once #include "detail/common.h" +#include "detail/typeid.h" +#include "cast.h" #include "operators.h" #include #include +#include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) @@ -636,19 +639,27 @@ auto map_if_insertion_operator(Class_ &cl, std::string const &name) "Return the canonical string representation of this map."); } -template +template struct keys_view { - Map ↦ + virtual size_t __len__() = 0; + virtual iterator __iter__() = 0; + virtual bool __contains__(const KeyType &k) = 0; + virtual bool __contains__(const object &k) = 0; + virtual ~keys_view() = default; }; -template +template struct values_view { - Map ↦ + virtual size_t __len__() = 0; + virtual iterator __iter__() = 0; + virtual ~values_view() = default; }; -template +template struct items_view { - Map ↦ + virtual size_t __len__() = 0; + virtual iterator __iter__() = 0; + virtual ~items_view() = default; }; PYBIND11_NAMESPACE_END(detail) @@ -657,9 +668,9 @@ template , typename... class_ bind_map(handle scope, const std::string &name, Args &&...args) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; - using KeysView = detail::keys_view; - using ValuesView = detail::values_view; - using ItemsView = detail::items_view; + using KeysView = detail::keys_view; + using ValuesView = detail::values_view; + using ItemsView = detail::items_view; using Class_ = class_; // If either type is a non-module-local bound type then make the map binding non-local as well; @@ -673,12 +684,48 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - class_ keys_view( - scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local)); - class_ values_view( - scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local)); - class_ items_view( - scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local)); + auto key_type_descr = detail::make_caster::name, + mapped_type_descr = detail::make_caster::name; + std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); + + // Wrap KeysView[KeyType] if it wasn't already wrapped + if (!detail::get_type_info(typeid(KeysView))) { + class_ keys_view( + scope, ("KeysView[" + key_type_name + "]").c_str(), pybind11::module_local(local)); + keys_view.def("__len__", &KeysView::__len__); + keys_view.def("__iter__", + &KeysView::__iter__, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + keys_view.def("__contains__", + static_cast(&KeysView::__contains__)); + // Fallback for when the object is not of the key type + keys_view.def("__contains__", + static_cast(&KeysView::__contains__)); + } + // Similarly for ValuesView: + if (!detail::get_type_info(typeid(ValuesView))) { + class_ values_view(scope, + ("ValuesView[" + mapped_type_name + "]").c_str(), + pybind11::module_local(local)); + values_view.def("__len__", &ValuesView::__len__); + values_view.def("__iter__", + &ValuesView::__iter__, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + } + // Similarly for ItemsView: + if (!detail::get_type_info(typeid(ItemsView))) { + class_ items_view( + scope, + ("ItemsView[" + key_type_name + ", " + mapped_type_name + "]").c_str(), + pybind11::module_local(local)); + items_view.def("__len__", &ItemsView::__len__); + items_view.def("__iter__", + &ItemsView::__iter__, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + } cl.def(init<>()); @@ -698,19 +745,51 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def( "keys", - [](Map &m) { return KeysView{m}; }, + [](Map &m) { + struct KeysViewImpl : public KeysView { + Map ↦ + KeysViewImpl(Map &map) : map(map) {} + size_t __len__() { return map.size(); } + iterator __iter__() { return make_key_iterator(map.begin(), map.end()); } + bool __contains__(const KeyType &k) { + auto it = map.find(k); + if (it == map.end()) { + return false; + } + return true; + } + bool __contains__(const object &) { return false; } + }; + return std::unique_ptr(new KeysViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "values", - [](Map &m) { return ValuesView{m}; }, + [](Map &m) { + struct ValuesViewImpl : public ValuesView { + Map ↦ + ValuesViewImpl(Map &map) : map(map) {} + size_t __len__() { return map.size(); } + iterator __iter__() { return make_value_iterator(map.begin(), map.end()); } + }; + return std::unique_ptr(new ValuesViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "items", - [](Map &m) { return ItemsView{m}; }, + [](Map &m) { + struct ItemsViewImpl : public ItemsView { + Map ↦ + ItemsViewImpl(Map &map) : map(map) {} + size_t __len__() { return map.size(); } + iterator __iter__() { return make_iterator(map.begin(), map.end()); } + }; + return std::unique_ptr(new ItemsViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); @@ -749,36 +828,6 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def("__len__", &Map::size); - keys_view.def("__len__", [](KeysView &view) { return view.map.size(); }); - keys_view.def( - "__iter__", - [](KeysView &view) { return make_key_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - keys_view.def("__contains__", [](KeysView &view, const KeyType &k) -> bool { - auto it = view.map.find(k); - if (it == view.map.end()) { - return false; - } - return true; - }); - // Fallback for when the object is not of the key type - keys_view.def("__contains__", [](KeysView &, const object &) -> bool { return false; }); - - values_view.def("__len__", [](ValuesView &view) { return view.map.size(); }); - values_view.def( - "__iter__", - [](ValuesView &view) { return make_value_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - - items_view.def("__len__", [](ItemsView &view) { return view.map.size(); }); - items_view.def( - "__iter__", - [](ItemsView &view) { return make_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - return cl; } From 6bbde1289c0e20ce1c12ab5721d33f600383a32c Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 01:44:26 +0200 Subject: [PATCH 02/20] We don't want to wrap different ValuesView objects for double values and const double, for example, as both wrappers will be named ValuesView[float] --- include/pybind11/stl_bind.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index ac90c895be..5c2b614e3d 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -668,9 +668,13 @@ template , typename... class_ bind_map(handle scope, const std::string &name, Args &&...args) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; - using KeysView = detail::keys_view; - using ValuesView = detail::values_view; - using ItemsView = detail::items_view; + using StrippedKeyType = + typename std::remove_cv::type>::type; + using StrippedMappedType = + typename std::remove_cv::type>::type; + using KeysView = detail::keys_view; + using ValuesView = detail::values_view; + using ItemsView = detail::items_view; using Class_ = class_; // If either type is a non-module-local bound type then make the map binding non-local as well; From f405e112e81c901c3a55db4c0cb3b7e4953a214c Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 01:47:31 +0200 Subject: [PATCH 03/20] Fallback to C++ names if key or values types are not wrapped --- include/pybind11/stl_bind.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 5c2b614e3d..48049fa70d 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -692,6 +692,15 @@ class_ bind_map(handle scope, const std::string &name, Args && mapped_type_descr = detail::make_caster::name; std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); + // If key type isn't properly wrapped, fall back to C++ names + if (key_type_name == "%") { + key_type_name = type_id(); + } + // Similarly for value type: + if (mapped_type_name == "%") { + mapped_type_name = type_id(); + } + // Wrap KeysView[KeyType] if it wasn't already wrapped if (!detail::get_type_info(typeid(KeysView))) { class_ keys_view( From 72a645e011acccb44325aa9d7a114c881d7c194c Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 01:58:20 +0200 Subject: [PATCH 04/20] Added a test for .keys(), .values() and .items() returning the same types for similarly-typed maps --- tests/test_stl_binders.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index d5e9ccced7..56dc3c1c57 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -309,3 +309,25 @@ def test_map_delitem(): del um["ua"] assert sorted(list(um)) == ["ub"] assert sorted(list(um.items())) == [("ub", 2.6)] + + +def test_map_view_types(): + map_string_double = m.MapStringDouble() + unordered_map_string_double = m.UnorderedMapStringDouble() + map_string_double_const = m.MapStringDoubleConst() + unordered_map_string_double_const = m.UnorderedMapStringDoubleConst() + + keys_type = type(map_string_double.keys()) + assert isinstance(unordered_map_string_double.keys(), keys_type) + assert isinstance(map_string_double_const.keys(), keys_type) + assert isinstance(unordered_map_string_double_const.keys(), keys_type) + + values_type = type(map_string_double.values()) + assert isinstance(unordered_map_string_double.values(), values_type) + assert isinstance(map_string_double_const.values(), values_type) + assert isinstance(unordered_map_string_double_const.values(), values_type) + + items_type = type(map_string_double.items()) + assert isinstance(unordered_map_string_double.items(), items_type) + assert isinstance(map_string_double_const.items(), items_type) + assert isinstance(unordered_map_string_double_const.items(), items_type) From 3e1b6536e2b69e6a35fa15fed29016bb80e2aa36 Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 02:50:40 +0200 Subject: [PATCH 05/20] Fixed wrong use of auto in a declarator list: the two descriptions might have different types --- include/pybind11/stl_bind.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 48049fa70d..39f131591e 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -688,8 +688,8 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - auto key_type_descr = detail::make_caster::name, - mapped_type_descr = detail::make_caster::name; + auto key_type_descr = detail::make_caster::name; + auto mapped_type_descr = detail::make_caster::name; std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); // If key type isn't properly wrapped, fall back to C++ names From c2846b4632904b0adb4fd3f683a05bd0b48c82e7 Mon Sep 17 00:00:00 2001 From: aimir Date: Wed, 23 Nov 2022 11:14:19 +0200 Subject: [PATCH 06/20] Fixes for clang-tidy issues: explicit single-argument constructor, using the 'override' keyword when overriding functions --- include/pybind11/stl_bind.h | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 39f131591e..d2927e17b1 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -761,17 +761,17 @@ class_ bind_map(handle scope, const std::string &name, Args && [](Map &m) { struct KeysViewImpl : public KeysView { Map ↦ - KeysViewImpl(Map &map) : map(map) {} - size_t __len__() { return map.size(); } - iterator __iter__() { return make_key_iterator(map.begin(), map.end()); } - bool __contains__(const KeyType &k) { + explicit KeysViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { return make_key_iterator(map.begin(), map.end()); } + bool __contains__(const KeyType &k) override { auto it = map.find(k); if (it == map.end()) { return false; } return true; } - bool __contains__(const object &) { return false; } + bool __contains__(const object &) override { return false; } }; return std::unique_ptr(new KeysViewImpl(m)); }, @@ -783,9 +783,11 @@ class_ bind_map(handle scope, const std::string &name, Args && [](Map &m) { struct ValuesViewImpl : public ValuesView { Map ↦ - ValuesViewImpl(Map &map) : map(map) {} - size_t __len__() { return map.size(); } - iterator __iter__() { return make_value_iterator(map.begin(), map.end()); } + explicit ValuesViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { + return make_value_iterator(map.begin(), map.end()); + } }; return std::unique_ptr(new ValuesViewImpl(m)); }, @@ -797,9 +799,9 @@ class_ bind_map(handle scope, const std::string &name, Args && [](Map &m) { struct ItemsViewImpl : public ItemsView { Map ↦ - ItemsViewImpl(Map &map) : map(map) {} - size_t __len__() { return map.size(); } - iterator __iter__() { return make_iterator(map.begin(), map.end()); } + explicit ItemsViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { return make_iterator(map.begin(), map.end()); } }; return std::unique_ptr(new ItemsViewImpl(m)); }, From dfc3ee180aa1f485ec675f2d6a2f4e7005fdc34c Mon Sep 17 00:00:00 2001 From: aimir Date: Wed, 23 Nov 2022 20:03:01 +0200 Subject: [PATCH 07/20] Bugfix for old versions of clang++, which seem to have trouble with the struct being defined inside a module, which was also needlessly ugly anyway --- include/pybind11/stl_bind.h | 76 ++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index d2927e17b1..7f0283c7df 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -662,6 +662,38 @@ struct items_view { virtual ~items_view() = default; }; +template +struct KeysViewImpl : public keys_view { + explicit KeysViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { return make_key_iterator(map.begin(), map.end()); } + bool __contains__(const typename Map::key_type &k) override { + auto it = map.find(k); + if (it == map.end()) { + return false; + } + return true; + } + bool __contains__(const object &) override { return false; } + Map ↦ +}; + +template +struct ValuesViewImpl : public values_view { + explicit ValuesViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { return make_value_iterator(map.begin(), map.end()); } + Map ↦ +}; + +template +struct ItemsViewImpl : public items_view { + explicit ItemsViewImpl(Map &map) : map(map) {} + size_t __len__() override { return map.size(); } + iterator __iter__() override { return make_iterator(map.begin(), map.end()); } + Map ↦ +}; + PYBIND11_NAMESPACE_END(detail) template , typename... Args> @@ -688,8 +720,8 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - auto key_type_descr = detail::make_caster::name; - auto mapped_type_descr = detail::make_caster::name; + static constexpr auto key_type_descr = detail::make_caster::name; + static constexpr auto mapped_type_descr = detail::make_caster::name; std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); // If key type isn't properly wrapped, fall back to C++ names @@ -758,53 +790,19 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def( "keys", - [](Map &m) { - struct KeysViewImpl : public KeysView { - Map ↦ - explicit KeysViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { return make_key_iterator(map.begin(), map.end()); } - bool __contains__(const KeyType &k) override { - auto it = map.find(k); - if (it == map.end()) { - return false; - } - return true; - } - bool __contains__(const object &) override { return false; } - }; - return std::unique_ptr(new KeysViewImpl(m)); - }, + [](Map &m) { return std::unique_ptr(new detail::KeysViewImpl(m)); }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "values", - [](Map &m) { - struct ValuesViewImpl : public ValuesView { - Map ↦ - explicit ValuesViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { - return make_value_iterator(map.begin(), map.end()); - } - }; - return std::unique_ptr(new ValuesViewImpl(m)); - }, + [](Map &m) { return std::unique_ptr(new detail::ValuesViewImpl(m)); }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "items", - [](Map &m) { - struct ItemsViewImpl : public ItemsView { - Map ↦ - explicit ItemsViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { return make_iterator(map.begin(), map.end()); } - }; - return std::unique_ptr(new ItemsViewImpl(m)); - }, + [](Map &m) { return std::unique_ptr(new detail::ItemsViewImpl(m)); }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); From 42d8694c5bca2315f21e641a516b240fcb239529 Mon Sep 17 00:00:00 2001 From: aimir Date: Wed, 23 Nov 2022 20:04:24 +0200 Subject: [PATCH 08/20] Bugfix for clang++, which doesn't have some of the names in runtime uness they are specified to be static --- include/pybind11/stl_bind.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 7f0283c7df..a0c56d0734 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -720,8 +720,8 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - static constexpr auto key_type_descr = detail::make_caster::name; - static constexpr auto mapped_type_descr = detail::make_caster::name; + auto key_type_descr = detail::make_caster::name; + auto mapped_type_descr = detail::make_caster::name; std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); // If key type isn't properly wrapped, fall back to C++ names From c558073a40adfdd8a0a6d3f1b699d6b79c0683bf Mon Sep 17 00:00:00 2001 From: aimir Date: Wed, 23 Nov 2022 20:24:08 +0200 Subject: [PATCH 09/20] A fix for clang-tidy performance-inefficient-string-concatenation issues - I personally think this looks uglier, but it's probably worth it for clang-tidy to be happy --- include/pybind11/stl_bind.h | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index a0c56d0734..43fe7eaa5c 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -662,8 +662,8 @@ struct items_view { virtual ~items_view() = default; }; -template -struct KeysViewImpl : public keys_view { +template +struct KeysViewImpl : public KeysView { explicit KeysViewImpl(Map &map) : map(map) {} size_t __len__() override { return map.size(); } iterator __iter__() override { return make_key_iterator(map.begin(), map.end()); } @@ -678,16 +678,16 @@ struct KeysViewImpl : public keys_view { Map ↦ }; -template -struct ValuesViewImpl : public values_view { +template +struct ValuesViewImpl : public ValuesView { explicit ValuesViewImpl(Map &map) : map(map) {} size_t __len__() override { return map.size(); } iterator __iter__() override { return make_value_iterator(map.begin(), map.end()); } Map ↦ }; -template -struct ItemsViewImpl : public items_view { +template +struct ItemsViewImpl : public ItemsView { explicit ItemsViewImpl(Map &map) : map(map) {} size_t __len__() override { return map.size(); } iterator __iter__() override { return make_iterator(map.begin(), map.end()); } @@ -763,7 +763,7 @@ class_ bind_map(handle scope, const std::string &name, Args && if (!detail::get_type_info(typeid(ItemsView))) { class_ items_view( scope, - ("ItemsView[" + key_type_name + ", " + mapped_type_name + "]").c_str(), + ("ItemsView[" + key_type_name + ", ").append(mapped_type_name + "]").c_str(), pybind11::module_local(local)); items_view.def("__len__", &ItemsView::__len__); items_view.def("__iter__", @@ -790,19 +790,25 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def( "keys", - [](Map &m) { return std::unique_ptr(new detail::KeysViewImpl(m)); }, + [](Map &m) { + return std::unique_ptr(new detail::KeysViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "values", - [](Map &m) { return std::unique_ptr(new detail::ValuesViewImpl(m)); }, + [](Map &m) { + return std::unique_ptr(new detail::ValuesViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "items", - [](Map &m) { return std::unique_ptr(new detail::ItemsViewImpl(m)); }, + [](Map &m) { + return std::unique_ptr(new detail::ItemsViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); From f0294f1c952b83a0b3faf1b38fafe54db4f56411 Mon Sep 17 00:00:00 2001 From: Amir Date: Wed, 23 Nov 2022 22:38:42 +0200 Subject: [PATCH 10/20] Possible fix for clang++ linking issues - make the descriptions static constexpr to make sure they are known before linking --- include/pybind11/stl_bind.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 43fe7eaa5c..c954740d56 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -720,8 +720,8 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - auto key_type_descr = detail::make_caster::name; - auto mapped_type_descr = detail::make_caster::name; + static constexpr auto key_type_descr = detail::make_caster::name; + static constexpr auto mapped_type_descr = detail::make_caster::name; std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); // If key type isn't properly wrapped, fall back to C++ names From ebf874df38c2f3f79f601133eb0cb90515da4620 Mon Sep 17 00:00:00 2001 From: Amir Date: Sun, 27 Nov 2022 01:17:33 +0200 Subject: [PATCH 11/20] Correct names for previously-wrapped types as keys/values of maps --- include/pybind11/stl_bind.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index c954740d56..abe81135a8 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -726,11 +726,23 @@ class_ bind_map(handle scope, const std::string &name, Args && // If key type isn't properly wrapped, fall back to C++ names if (key_type_name == "%") { - key_type_name = type_id(); + if (auto *key_info = detail::get_type_info(typeid(KeyType))) { + handle th((PyObject *) tinfo->type); + key_type_name = th.attr("__module__").cast() + "." + + th.attr("__qualname__").cast(); + } else { + key_type_name = type_id(); + } } // Similarly for value type: if (mapped_type_name == "%") { - mapped_type_name = type_id(); + if (auto *mapped_info = detail::get_type_info(typeid(MappedType))) { + handle th((PyObject *) tinfo->type); + mapped_type_name = th.attr("__module__").cast() + "." + + th.attr("__qualname__").cast(); + } else { + mapped_type_name = type_id(); + } } // Wrap KeysView[KeyType] if it wasn't already wrapped From 24019aed16ce6a2b4ab6cc0521a80f4fb8f34fa2 Mon Sep 17 00:00:00 2001 From: Amir Date: Sun, 27 Nov 2022 01:47:18 +0200 Subject: [PATCH 12/20] Bugfix - typo in type info names which caused things to segfault --- include/pybind11/stl_bind.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index abe81135a8..ab7b382fe6 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -727,7 +727,7 @@ class_ bind_map(handle scope, const std::string &name, Args && // If key type isn't properly wrapped, fall back to C++ names if (key_type_name == "%") { if (auto *key_info = detail::get_type_info(typeid(KeyType))) { - handle th((PyObject *) tinfo->type); + handle th((PyObject *) key_info->type); key_type_name = th.attr("__module__").cast() + "." + th.attr("__qualname__").cast(); } else { @@ -737,7 +737,7 @@ class_ bind_map(handle scope, const std::string &name, Args && // Similarly for value type: if (mapped_type_name == "%") { if (auto *mapped_info = detail::get_type_info(typeid(MappedType))) { - handle th((PyObject *) tinfo->type); + handle th((PyObject *) mapped_info->type); mapped_type_name = th.attr("__module__").cast() + "." + th.attr("__qualname__").cast(); } else { From ddcc4e2bf14bd4340d9477782264f0da2d013f19 Mon Sep 17 00:00:00 2001 From: aimir <48388610+aimir@users.noreply.github.com> Date: Sun, 27 Nov 2022 01:58:26 +0200 Subject: [PATCH 13/20] Apply suggestions from code review Co-authored-by: Aaron Gokaslan --- include/pybind11/stl_bind.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index ab7b382fe6..a04d6db700 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -728,7 +728,7 @@ class_ bind_map(handle scope, const std::string &name, Args && if (key_type_name == "%") { if (auto *key_info = detail::get_type_info(typeid(KeyType))) { handle th((PyObject *) key_info->type); - key_type_name = th.attr("__module__").cast() + "." + key_type_name = th.attr("__module__").cast() + '.' + th.attr("__qualname__").cast(); } else { key_type_name = type_id(); @@ -738,7 +738,7 @@ class_ bind_map(handle scope, const std::string &name, Args && if (mapped_type_name == "%") { if (auto *mapped_info = detail::get_type_info(typeid(MappedType))) { handle th((PyObject *) mapped_info->type); - mapped_type_name = th.attr("__module__").cast() + "." + mapped_type_name = th.attr("__module__").cast() + '.' + th.attr("__qualname__").cast(); } else { mapped_type_name = type_id(); From 21ec3ec4f4bb110a64812aa4de7ed800e75842c3 Mon Sep 17 00:00:00 2001 From: aimir Date: Mon, 28 Nov 2022 12:45:13 +0200 Subject: [PATCH 14/20] Use detail::remove_cvref_t instead of doing remove_cv and remove_reference separately --- include/pybind11/stl_bind.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index a04d6db700..e522685d10 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -700,10 +700,8 @@ template , typename... class_ bind_map(handle scope, const std::string &name, Args &&...args) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; - using StrippedKeyType = - typename std::remove_cv::type>::type; - using StrippedMappedType = - typename std::remove_cv::type>::type; + using StrippedKeyType = detail::remove_cvref_t; + using StrippedMappedType = detail::remove_cvref_t; using KeysView = detail::keys_view; using ValuesView = detail::values_view; using ItemsView = detail::items_view; From 8abc0aec159080bb08324f93cde08ad75fc4842d Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 2 Dec 2022 19:10:13 +0200 Subject: [PATCH 15/20] Avoid names with double underscore, as they are reserved --- include/pybind11/stl_bind.h | 48 ++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index e522685d10..5082556fad 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -641,56 +641,56 @@ auto map_if_insertion_operator(Class_ &cl, std::string const &name) template struct keys_view { - virtual size_t __len__() = 0; - virtual iterator __iter__() = 0; - virtual bool __contains__(const KeyType &k) = 0; - virtual bool __contains__(const object &k) = 0; + virtual size_t len() = 0; + virtual iterator iter() = 0; + virtual bool contains(const KeyType &k) = 0; + virtual bool contains(const object &k) = 0; virtual ~keys_view() = default; }; template struct values_view { - virtual size_t __len__() = 0; - virtual iterator __iter__() = 0; + virtual size_t len() = 0; + virtual iterator iter() = 0; virtual ~values_view() = default; }; template struct items_view { - virtual size_t __len__() = 0; - virtual iterator __iter__() = 0; + virtual size_t len() = 0; + virtual iterator iter() = 0; virtual ~items_view() = default; }; template struct KeysViewImpl : public KeysView { explicit KeysViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { return make_key_iterator(map.begin(), map.end()); } - bool __contains__(const typename Map::key_type &k) override { + size_t len() override { return map.size(); } + iterator iter() override { return make_key_iterator(map.begin(), map.end()); } + bool contains(const typename Map::key_type &k) override { auto it = map.find(k); if (it == map.end()) { return false; } return true; } - bool __contains__(const object &) override { return false; } + bool contains(const object &) override { return false; } Map ↦ }; template struct ValuesViewImpl : public ValuesView { explicit ValuesViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { return make_value_iterator(map.begin(), map.end()); } + size_t len() override { return map.size(); } + iterator iter() override { return make_value_iterator(map.begin(), map.end()); } Map ↦ }; template struct ItemsViewImpl : public ItemsView { explicit ItemsViewImpl(Map &map) : map(map) {} - size_t __len__() override { return map.size(); } - iterator __iter__() override { return make_iterator(map.begin(), map.end()); } + size_t len() override { return map.size(); } + iterator iter() override { return make_iterator(map.begin(), map.end()); } Map ↦ }; @@ -747,25 +747,25 @@ class_ bind_map(handle scope, const std::string &name, Args && if (!detail::get_type_info(typeid(KeysView))) { class_ keys_view( scope, ("KeysView[" + key_type_name + "]").c_str(), pybind11::module_local(local)); - keys_view.def("__len__", &KeysView::__len__); + keys_view.def("__len__", &KeysView::len); keys_view.def("__iter__", - &KeysView::__iter__, + &KeysView::iter, keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ ); keys_view.def("__contains__", - static_cast(&KeysView::__contains__)); + static_cast(&KeysView::contains)); // Fallback for when the object is not of the key type keys_view.def("__contains__", - static_cast(&KeysView::__contains__)); + static_cast(&KeysView::contains)); } // Similarly for ValuesView: if (!detail::get_type_info(typeid(ValuesView))) { class_ values_view(scope, ("ValuesView[" + mapped_type_name + "]").c_str(), pybind11::module_local(local)); - values_view.def("__len__", &ValuesView::__len__); + values_view.def("__len__", &ValuesView::len); values_view.def("__iter__", - &ValuesView::__iter__, + &ValuesView::iter, keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ ); } @@ -775,9 +775,9 @@ class_ bind_map(handle scope, const std::string &name, Args && scope, ("ItemsView[" + key_type_name + ", ").append(mapped_type_name + "]").c_str(), pybind11::module_local(local)); - items_view.def("__len__", &ItemsView::__len__); + items_view.def("__len__", &ItemsView::len); items_view.def("__iter__", - &ItemsView::__iter__, + &ItemsView::iter, keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ ); } From 60d754f119f2bdc968c155c5bee75de13746264b Mon Sep 17 00:00:00 2001 From: Amir Date: Sun, 4 Dec 2022 22:12:22 +0200 Subject: [PATCH 16/20] Improved testing for KeysView, ValuesView and ItemsView: check type names + stricter asserts --- tests/test_stl_binders.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 56dc3c1c57..9eb906f065 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -317,17 +317,21 @@ def test_map_view_types(): map_string_double_const = m.MapStringDoubleConst() unordered_map_string_double_const = m.UnorderedMapStringDoubleConst() + assert map_string_double.keys().__class__.__name__ == "KeysView[str]" + assert map_string_double.values().__class__.__name__ == "ValuesView[float]" + assert map_string_double.items().__class__.__name__ == "ItemsView[str, float]" + keys_type = type(map_string_double.keys()) - assert isinstance(unordered_map_string_double.keys(), keys_type) - assert isinstance(map_string_double_const.keys(), keys_type) - assert isinstance(unordered_map_string_double_const.keys(), keys_type) + assert type(unordered_map_string_double.keys()) is keys_type + assert type(map_string_double_const.keys()) is keys_type + assert type(unordered_map_string_double_const.keys()) is keys_type values_type = type(map_string_double.values()) - assert isinstance(unordered_map_string_double.values(), values_type) - assert isinstance(map_string_double_const.values(), values_type) - assert isinstance(unordered_map_string_double_const.values(), values_type) + assert type(unordered_map_string_double.values()) is values_type + assert type(map_string_double_const.values()) is values_type + assert type(unordered_map_string_double_const.values()) is values_type items_type = type(map_string_double.items()) - assert isinstance(unordered_map_string_double.items(), items_type) - assert isinstance(map_string_double_const.items(), items_type) - assert isinstance(unordered_map_string_double_const.items(), items_type) + assert type(unordered_map_string_double.items()) is items_type + assert type(map_string_double_const.items()) is items_type + assert type(unordered_map_string_double_const.items()) is items_type From e054752bb9aa9288bcdc772d1ffc05f467da6991 Mon Sep 17 00:00:00 2001 From: Amir Date: Sun, 4 Dec 2022 23:27:47 +0200 Subject: [PATCH 17/20] Moved description logic to helper function in type_caster_base.h --- include/pybind11/detail/type_caster_base.h | 9 +++++++++ include/pybind11/stl_bind.h | 18 +++--------------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 21f69c289f..5569d7a06c 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -1006,5 +1006,14 @@ class type_caster_base : public type_caster_generic { static Constructor make_move_constructor(...) { return nullptr; } }; +PYBIND11_NOINLINE std::string type_info_description(const std::type_info& ti) { + if (auto *type_data = get_type_info(ti)) { + handle th((PyObject *) type_data->type); + return th.attr("__module__").cast() + '.' + th.attr("__qualname__").cast(); + } else { + return clean_type_id(ti.name()); + } +} + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 5082556fad..f520651191 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -10,7 +10,7 @@ #pragma once #include "detail/common.h" -#include "detail/typeid.h" +#include "detail/type_caster_base.h" #include "cast.h" #include "operators.h" @@ -724,23 +724,11 @@ class_ bind_map(handle scope, const std::string &name, Args && // If key type isn't properly wrapped, fall back to C++ names if (key_type_name == "%") { - if (auto *key_info = detail::get_type_info(typeid(KeyType))) { - handle th((PyObject *) key_info->type); - key_type_name = th.attr("__module__").cast() + '.' - + th.attr("__qualname__").cast(); - } else { - key_type_name = type_id(); - } + key_type_name = detail::type_info_description(typeid(KeyType)); } // Similarly for value type: if (mapped_type_name == "%") { - if (auto *mapped_info = detail::get_type_info(typeid(MappedType))) { - handle th((PyObject *) mapped_info->type); - mapped_type_name = th.attr("__module__").cast() + '.' - + th.attr("__qualname__").cast(); - } else { - mapped_type_name = type_id(); - } + mapped_type_name = detail::type_info_description(typeid(MappedType)); } // Wrap KeysView[KeyType] if it wasn't already wrapped From b4041b86425e5fd239e067bf746d7cb45d516505 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 4 Dec 2022 21:28:21 +0000 Subject: [PATCH 18/20] style: pre-commit fixes --- include/pybind11/detail/type_caster_base.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 5569d7a06c..02d2fa9ba8 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -1006,10 +1006,11 @@ class type_caster_base : public type_caster_generic { static Constructor make_move_constructor(...) { return nullptr; } }; -PYBIND11_NOINLINE std::string type_info_description(const std::type_info& ti) { +PYBIND11_NOINLINE std::string type_info_description(const std::type_info &ti) { if (auto *type_data = get_type_info(ti)) { handle th((PyObject *) type_data->type); - return th.attr("__module__").cast() + '.' + th.attr("__qualname__").cast(); + return th.attr("__module__").cast() + '.' + + th.attr("__qualname__").cast(); } else { return clean_type_id(ti.name()); } From 3a3b9f24193bd1a91a078a452be0772ec1fcd5aa Mon Sep 17 00:00:00 2001 From: Amir Date: Sun, 4 Dec 2022 23:41:30 +0200 Subject: [PATCH 19/20] Fix a clang-tidy issue: do not use 'else' after 'return' --- include/pybind11/detail/type_caster_base.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 02d2fa9ba8..0b710d7e4c 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -1011,9 +1011,8 @@ PYBIND11_NOINLINE std::string type_info_description(const std::type_info &ti) { handle th((PyObject *) type_data->type); return th.attr("__module__").cast() + '.' + th.attr("__qualname__").cast(); - } else { - return clean_type_id(ti.name()); } + return clean_type_id(ti.name()); } PYBIND11_NAMESPACE_END(detail) From 4525c5d1363631c782e84111af9ff588096292fb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 8 Dec 2022 22:41:37 -0800 Subject: [PATCH 20/20] Apply suggestion by @Skylion007, with additional trivial simplification. --- include/pybind11/stl_bind.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index f520651191..0c634597ec 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -667,13 +667,7 @@ struct KeysViewImpl : public KeysView { explicit KeysViewImpl(Map &map) : map(map) {} size_t len() override { return map.size(); } iterator iter() override { return make_key_iterator(map.begin(), map.end()); } - bool contains(const typename Map::key_type &k) override { - auto it = map.find(k); - if (it == map.end()) { - return false; - } - return true; - } + bool contains(const typename Map::key_type &k) override { return map.find(k) != map.end(); } bool contains(const object &) override { return false; } Map ↦ };