Skip to content

Commit

Permalink
Correct class names for KeysView, ValuesView and ItemsView in bind_map (
Browse files Browse the repository at this point in the history 
pybind#4353)

* Create templated abstract classes KeysView, ValuesView and ItemsView, and implement them on-the-fly when wrapping any specific map type

* 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]

* Fallback to C++ names if key or values types are not wrapped

* Added a test for .keys(), .values() and .items() returning the same types for similarly-typed maps

* Fixed wrong use of auto in a declarator list: the two descriptions might have different types

* Fixes for clang-tidy issues: explicit single-argument constructor, using the 'override' keyword when overriding functions

* 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

* Bugfix for clang++, which doesn't have some of the names in runtime uness they are specified to be static

* 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

* Possible fix for clang++ linking issues - make the descriptions static constexpr to make sure they are known before linking

* Correct names for previously-wrapped types as keys/values of maps

* Bugfix - typo in type info names which caused things to segfault

* Apply suggestions from code review

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

* Use detail::remove_cvref_t instead of doing remove_cv and remove_reference separately

* Avoid names with double underscore, as they are reserved

* Improved testing for KeysView, ValuesView and ItemsView: check type names + stricter asserts

* Moved description logic to helper function in type_caster_base.h

* style: pre-commit fixes

* Fix a clang-tidy issue: do not use 'else' after 'return'

* Apply suggestion by @Skylion007, with additional trivial simplification.

Co-authored-by: Amir <aimir@local>
Co-authored-by: aimir <aimir@localhost>
Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rwgk@google.com>
  • Loading branch information
@aimir @Skylion007 @pre-commit-ci
6 people authored and Taiju Yamada committed Jun 12, 2023
1 parent bb14240 commit 16e2b9b
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 47 deletions.
9 changes: 9 additions & 0 deletions include/pybind11/detail/type_caster_base.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1079,5 +1079,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<std::string>() + '.'
+ th.attr("__qualname__").cast<std::string>();
}
return clean_type_id(ti.name());
}

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
154 changes: 107 additions & 47 deletions include/pybind11/stl_bind.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
#pragma once

#include "detail/common.h"
#include "detail/type_caster_base.h"
#include "cast.h"
#include "operators.h"

#include <algorithm>
#include <sstream>
#include <type_traits>

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)
Expand Down Expand Up @@ -636,18 +639,52 @@ auto map_if_insertion_operator(Class_ &cl, std::string const &name)
"Return the canonical string representation of this map.");
}

template <typename Map>
template <typename KeyType>
struct keys_view {
Map &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 <typename Map>
template <typename MappedType>
struct values_view {
Map &map;
virtual size_t len() = 0;
virtual iterator iter() = 0;
virtual ~values_view() = default;
};

template <typename Map>
template <typename KeyType, typename MappedType>
struct items_view {
virtual size_t len() = 0;
virtual iterator iter() = 0;
virtual ~items_view() = default;
};

template <typename Map, typename KeysView>
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 { return map.find(k) != map.end(); }
bool contains(const object &) override { return false; }
Map &map;
};

template <typename Map, typename ValuesView>
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 &map;
};

template <typename Map, typename ItemsView>
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()); }
Map &map;
};

Expand All @@ -657,9 +694,11 @@ template <typename Map, typename holder_type = std::unique_ptr<Map>, typename...
class_<Map, holder_type> 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<Map>;
using ValuesView = detail::values_view<Map>;
using ItemsView = detail::items_view<Map>;
using StrippedKeyType = detail::remove_cvref_t<KeyType>;
using StrippedMappedType = detail::remove_cvref_t<MappedType>;
using KeysView = detail::keys_view<StrippedKeyType>;
using ValuesView = detail::values_view<StrippedMappedType>;
using ItemsView = detail::items_view<StrippedKeyType, StrippedMappedType>;
using Class_ = class_<Map, holder_type>;

// If either type is a non-module-local bound type then make the map binding non-local as well;
Expand All @@ -673,12 +712,57 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&
}

Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward<Args>(args)...);
class_<KeysView> keys_view(
scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local));
class_<ValuesView> values_view(
scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local));
class_<ItemsView> items_view(
scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local));
static constexpr auto key_type_descr = detail::make_caster<KeyType>::name;
static constexpr auto mapped_type_descr = detail::make_caster<MappedType>::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 = detail::type_info_description(typeid(KeyType));
}
// Similarly for value type:
if (mapped_type_name == "%") {
mapped_type_name = detail::type_info_description(typeid(MappedType));
}

// Wrap KeysView[KeyType] if it wasn't already wrapped
if (!detail::get_type_info(typeid(KeysView))) {
class_<KeysView> 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<bool (KeysView::*)(const KeyType &)>(&KeysView::contains));
// Fallback for when the object is not of the key type
keys_view.def("__contains__",
static_cast<bool (KeysView::*)(const object &)>(&KeysView::contains));
}
// Similarly for ValuesView:
if (!detail::get_type_info(typeid(ValuesView))) {
class_<ValuesView> 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_<ItemsView> items_view(
scope,
("ItemsView[" + key_type_name + ", ").append(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<>());

Expand All @@ -698,19 +782,25 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args &&

cl.def(
"keys",
[](Map &m) { return KeysView{m}; },
[](Map &m) {
return std::unique_ptr<KeysView>(new detail::KeysViewImpl<Map, KeysView>(m));
},
keep_alive<0, 1>() /* Essential: keep map alive while view exists */
);

cl.def(
"values",
[](Map &m) { return ValuesView{m}; },
[](Map &m) {
return std::unique_ptr<ValuesView>(new detail::ValuesViewImpl<Map, ValuesView>(m));
},
keep_alive<0, 1>() /* Essential: keep map alive while view exists */
);

cl.def(
"items",
[](Map &m) { return ItemsView{m}; },
[](Map &m) {
return std::unique_ptr<ItemsView>(new detail::ItemsViewImpl<Map, ItemsView>(m));
},
keep_alive<0, 1>() /* Essential: keep map alive while view exists */
);

Expand Down Expand Up @@ -749,36 +839,6 @@ class_<Map, holder_type> 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;
}

Expand Down
26 changes: 26 additions & 0 deletions tests/test_stl_binders.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,3 +316,29 @@ 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()

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 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 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 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

0 comments on commit 16e2b9b

Please sign in to comment.