From e8280318664b9ff27a8bc306a13ef2d5ebc200a2 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 24 Apr 2022 21:54:59 +0100 Subject: [PATCH 1/8] Add frozenset, and allow it cast to std::set For the reverse direction, std::set still casts to set. This is in concordance with the behavior for sequence containers, where e.g. tuple casts to std::vector but std::vector casts to list. Extracted from #3886. --- include/pybind11/pytypes.h | 33 +++++++++++++++++++++++++-------- include/pybind11/stl.h | 4 ++-- tests/test_stl.py | 1 + 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 324fa932f1..fa2475d6ab 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1784,24 +1784,41 @@ class kwargs : public dict { PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check) }; -class set : public object { +class set_base : public object { +protected: + PYBIND11_OBJECT(set_base, object, PyAnySet_Check) + +public: + size_t size() const { return (size_t) PySet_Size(m_ptr); } + bool empty() const { return size() == 0; } + template + bool contains(T &&val) const { + return PySet_Contains(m_ptr, detail::object_or_cast(std::forward(val)).ptr()) == 1; + } +}; + +class set : public set_base { public: - PYBIND11_OBJECT_CVT(set, object, PySet_Check, PySet_New) - set() : object(PySet_New(nullptr), stolen_t{}) { + PYBIND11_OBJECT_CVT(set, set_base, PySet_Check, PySet_New) + set() : set_base(PySet_New(nullptr), stolen_t{}) { if (!m_ptr) { pybind11_fail("Could not allocate set object!"); } } - size_t size() const { return (size_t) PySet_Size(m_ptr); } - bool empty() const { return size() == 0; } template bool add(T &&val) /* py-non-const */ { return PySet_Add(m_ptr, detail::object_or_cast(std::forward(val)).ptr()) == 0; } void clear() /* py-non-const */ { PySet_Clear(m_ptr); } - template - bool contains(T &&val) const { - return PySet_Contains(m_ptr, detail::object_or_cast(std::forward(val)).ptr()) == 1; +}; + +class frozenset : public set_base { +public: + PYBIND11_OBJECT_CVT(frozenset, set_base, PyFrozenSet_Check, PyFrozenSet_New) + frozenset() : set_base(PyFrozenSet_New(nullptr), stolen_t{}) { + if (!m_ptr) { + pybind11_fail("Could not allocate frozenset object!"); + } } }; diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 51b57a92ba..838c2dc341 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -55,10 +55,10 @@ struct set_caster { using key_conv = make_caster; bool load(handle src, bool convert) { - if (!isinstance(src)) { + if (!isinstance(src)) { return false; } - auto s = reinterpret_borrow(src); + auto s = reinterpret_borrow(src); value.clear(); for (auto entry : s) { key_conv conv; diff --git a/tests/test_stl.py b/tests/test_stl.py index 3dc55230ab..2c858e3f5c 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -70,6 +70,7 @@ def test_set(doc): assert s == {"key1", "key2"} s.add("key3") assert m.load_set(s) + assert m.load_set(frozenset(s)) assert doc(m.cast_set) == "cast_set() -> Set[str]" assert doc(m.load_set) == "load_set(arg0: Set[str]) -> bool" From f2db7bb14e6ef601c4085b791027c7ca9a5c67ea Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 24 Apr 2022 21:56:33 +0100 Subject: [PATCH 2/8] Rename set_base to any_set to match Python C API since this will be part of pybind11 public API --- include/pybind11/pytypes.h | 16 ++++++++-------- include/pybind11/stl.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index fa2475d6ab..0322235c92 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1784,9 +1784,9 @@ class kwargs : public dict { PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check) }; -class set_base : public object { +class any_set : public object { protected: - PYBIND11_OBJECT(set_base, object, PyAnySet_Check) + PYBIND11_OBJECT(any_set, object, PyAnySet_Check) public: size_t size() const { return (size_t) PySet_Size(m_ptr); } @@ -1797,10 +1797,10 @@ class set_base : public object { } }; -class set : public set_base { +class set : public any_set { public: - PYBIND11_OBJECT_CVT(set, set_base, PySet_Check, PySet_New) - set() : set_base(PySet_New(nullptr), stolen_t{}) { + PYBIND11_OBJECT_CVT(set, any_set, PySet_Check, PySet_New) + set() : any_set(PySet_New(nullptr), stolen_t{}) { if (!m_ptr) { pybind11_fail("Could not allocate set object!"); } @@ -1812,10 +1812,10 @@ class set : public set_base { void clear() /* py-non-const */ { PySet_Clear(m_ptr); } }; -class frozenset : public set_base { +class frozenset : public any_set { public: - PYBIND11_OBJECT_CVT(frozenset, set_base, PyFrozenSet_Check, PyFrozenSet_New) - frozenset() : set_base(PyFrozenSet_New(nullptr), stolen_t{}) { + PYBIND11_OBJECT_CVT(frozenset, any_set, PyFrozenSet_Check, PyFrozenSet_New) + frozenset() : any_set(PyFrozenSet_New(nullptr), stolen_t{}) { if (!m_ptr) { pybind11_fail("Could not allocate frozenset object!"); } diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 838c2dc341..0d67176bdf 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -55,10 +55,10 @@ struct set_caster { using key_conv = make_caster; bool load(handle src, bool convert) { - if (!isinstance(src)) { + if (!isinstance(src)) { return false; } - auto s = reinterpret_borrow(src); + auto s = reinterpret_borrow(src); value.clear(); for (auto entry : s) { key_conv conv; From ef92aa5e17aff1e587163779f89825adaf4c8fa5 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 1 May 2022 12:17:40 +0100 Subject: [PATCH 3/8] PR: static_cast, anyset --- include/pybind11/pytypes.h | 18 +++++++++--------- include/pybind11/stl.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 0322235c92..963cc564e2 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1784,12 +1784,12 @@ class kwargs : public dict { PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check) }; -class any_set : public object { +class anyset : public object { protected: - PYBIND11_OBJECT(any_set, object, PyAnySet_Check) + PYBIND11_OBJECT(anyset, object, PyAnySet_Check) public: - size_t size() const { return (size_t) PySet_Size(m_ptr); } + size_t size() const { return static_cast(PySet_Size(m_ptr)); } bool empty() const { return size() == 0; } template bool contains(T &&val) const { @@ -1797,10 +1797,10 @@ class any_set : public object { } }; -class set : public any_set { +class set : public anyset { public: - PYBIND11_OBJECT_CVT(set, any_set, PySet_Check, PySet_New) - set() : any_set(PySet_New(nullptr), stolen_t{}) { + PYBIND11_OBJECT_CVT(set, anyset, PySet_Check, PySet_New) + set() : anyset(PySet_New(nullptr), stolen_t{}) { if (!m_ptr) { pybind11_fail("Could not allocate set object!"); } @@ -1812,10 +1812,10 @@ class set : public any_set { void clear() /* py-non-const */ { PySet_Clear(m_ptr); } }; -class frozenset : public any_set { +class frozenset : public anyset { public: - PYBIND11_OBJECT_CVT(frozenset, any_set, PyFrozenSet_Check, PyFrozenSet_New) - frozenset() : any_set(PyFrozenSet_New(nullptr), stolen_t{}) { + PYBIND11_OBJECT_CVT(frozenset, anyset, PyFrozenSet_Check, PyFrozenSet_New) + frozenset() : anyset(PyFrozenSet_New(nullptr), stolen_t{}) { if (!m_ptr) { pybind11_fail("Could not allocate frozenset object!"); } diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 0d67176bdf..625fb210fc 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -55,10 +55,10 @@ struct set_caster { using key_conv = make_caster; bool load(handle src, bool convert) { - if (!isinstance(src)) { + if (!isinstance(src)) { return false; } - auto s = reinterpret_borrow(src); + auto s = reinterpret_borrow(src); value.clear(); for (auto entry : s) { key_conv conv; From 736f293de6e966cca784a8277db9cc08fc74677f Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 1 May 2022 12:34:58 +0100 Subject: [PATCH 4/8] Add tests for frozenset and rename anyset methods --- tests/test_pytypes.cpp | 19 ++++++++++++----- tests/test_pytypes.py | 48 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index d1e9b81a73..9347385789 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -75,7 +75,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("get_none", [] { return py::none(); }); m.def("print_none", [](const py::none &none) { py::print("none: {}"_s.format(none)); }); - // test_set + // test_set, test_frozenset m.def("get_set", []() { py::set set; set.add(py::str("key1")); @@ -83,14 +83,23 @@ TEST_SUBMODULE(pytypes, m) { set.add(std::string("key3")); return set; }); - m.def("print_set", [](const py::set &set) { + m.def("get_frozenset", []() { + py::set set; + set.add(py::str("key1")); + set.add("key2"); + set.add(std::string("key3")); + return py::frozenset(set); + }); + m.def("print_anyset", [](const py::anyset &set) { for (auto item : set) { py::print("key:", item); } }); - m.def("set_contains", - [](const py::set &set, const py::object &key) { return set.contains(key); }); - m.def("set_contains", [](const py::set &set, const char *key) { return set.contains(key); }); + m.def("anyset_size", [](const py::anyset &set) { return set.size(); }); + m.def("anyset_empty", [](const py::anyset &set) { return set.empty(); }); + m.def("anyset_contains", + [](const py::anyset &set, const py::object &key) { return set.contains(key); }); + m.def("anyset_contains", [](const py::anyset &set, const char *key) { return set.contains(key); }); // test_dict m.def("get_dict", []() { return py::dict("key"_a = "value"); }); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 5c715ada6b..65233f7f97 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -66,11 +66,12 @@ def test_none(capture, doc): def test_set(capture, doc): s = m.get_set() + assert isinstance(s, set) assert s == {"key1", "key2", "key3"} with capture: s.add("key4") - m.print_set(s) + m.print_anyset(s) assert ( capture.unordered == """ @@ -81,12 +82,47 @@ def test_set(capture, doc): """ ) - assert not m.set_contains(set(), 42) - assert m.set_contains({42}, 42) - assert m.set_contains({"foo"}, "foo") + assert m.anyset_size(set() == 0 + assert m.anyset_size(s) == 4 - assert doc(m.get_list) == "get_list() -> list" - assert doc(m.print_list) == "print_list(arg0: list) -> None" + assert m.anyset_empty(set()) + assert not m.anyset_empty({42}) + + assert not m.anyset_contains(set(), 42) + assert m.anyset_contains({42}, 42) + assert m.anyset_contains({"foo"}, "foo") + + assert doc(m.get_set) == "get_set() -> set" + assert doc(m.print_anyset) == "print_anyset(arg0: set) -> None" + + +def test_frozenset(capture, doc): + s = m.get_frozenset() + assert isinstance(s, frozenset) + assert s == frozenset({"key1", "key2", "key3"}) + + with capture: + m.print_anyset(s) + assert ( + capture.unordered + == """ + key: key1 + key: key2 + key: key3 + """ + ) + + assert m.anyset_size(frozenset()) == 0 + assert m.anyset_size(s) == 3 + + assert m.anyset_empty(frozenset()) + assert not m.anyset_empty(frozenset({42})) + + assert not m.anyset_contains(frozenset(), 42) + assert m.anyset_contains(frozenset({42}), 42) + assert m.anyset_contains(frozenset({"foo"}), "foo") + + assert doc(m.get_frozenset) == "get_frozenset() -> frozenset" def test_dict(capture, doc): From faf8a519a2e6b92c59dbee62271c0c75effbc29b Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 1 May 2022 16:01:06 +0100 Subject: [PATCH 5/8] Remove frozenset default ctor, add tests Making frozenset non-default constructible means that we need to adjust pyobject_caster to not require that its value is default constructible, by initializing value to a nil handle. This also allows writing C++ functions taking anyset, and is arguably a performance improvement, since there is no need to allocate an object that will just be replaced by load. Add some more tests, including anyset::empty, anyset::size, set::add and set::clear. --- include/pybind11/cast.h | 6 ++++++ include/pybind11/pytypes.h | 5 ----- tests/test_pytypes.cpp | 4 ++++ tests/test_pytypes.py | 19 ++++++++----------- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e8128710e2..7d1e943dee 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -907,6 +907,12 @@ struct handle_type_name { template struct pyobject_caster { + template ::value, int> = 0> + pyobject_caster() : value() {} + + template ::value, int> = 0> + pyobject_caster() : value(reinterpret_steal(handle())) {} + template ::value, int> = 0> bool load(handle src, bool /* convert */) { value = src; diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 963cc564e2..a79b0caadf 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1815,11 +1815,6 @@ class set : public anyset { class frozenset : public anyset { public: PYBIND11_OBJECT_CVT(frozenset, anyset, PyFrozenSet_Check, PyFrozenSet_New) - frozenset() : anyset(PyFrozenSet_New(nullptr), stolen_t{}) { - if (!m_ptr) { - pybind11_fail("Could not allocate frozenset object!"); - } - } }; class function : public object { diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 9347385789..1f26625d14 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -100,6 +100,8 @@ TEST_SUBMODULE(pytypes, m) { m.def("anyset_contains", [](const py::anyset &set, const py::object &key) { return set.contains(key); }); m.def("anyset_contains", [](const py::anyset &set, const char *key) { return set.contains(key); }); + m.def("set_add", [](py::set &set, const py::object &key) { set.add(key); }); + m.def("set_clear", [](py::set &set) { set.clear(); }); // test_dict m.def("get_dict", []() { return py::dict("key"_a = "value"); }); @@ -319,6 +321,7 @@ TEST_SUBMODULE(pytypes, m) { "list"_a = py::list(d["list"]), "dict"_a = py::dict(d["dict"]), "set"_a = py::set(d["set"]), + "frozenset"_a = py::frozenset(d["frozenset"]), "memoryview"_a = py::memoryview(d["memoryview"])); }); @@ -334,6 +337,7 @@ TEST_SUBMODULE(pytypes, m) { "list"_a = d["list"].cast(), "dict"_a = d["dict"].cast(), "set"_a = d["set"].cast(), + "frozenset"_a = d["frozenset"].cast(), "memoryview"_a = d["memoryview"].cast()); }); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 65233f7f97..a6adfdddad 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -69,8 +69,8 @@ def test_set(capture, doc): assert isinstance(s, set) assert s == {"key1", "key2", "key3"} + s.add("key4") with capture: - s.add("key4") m.print_anyset(s) assert ( capture.unordered @@ -82,18 +82,18 @@ def test_set(capture, doc): """ ) - assert m.anyset_size(set() == 0 - assert m.anyset_size(s) == 4 + m.set_add(s, "key5") + assert m.anyset_size(s) == 5 - assert m.anyset_empty(set()) - assert not m.anyset_empty({42}) + m.set_clear(s) + assert m.anyset_empty(s) assert not m.anyset_contains(set(), 42) assert m.anyset_contains({42}, 42) assert m.anyset_contains({"foo"}, "foo") assert doc(m.get_set) == "get_set() -> set" - assert doc(m.print_anyset) == "print_anyset(arg0: set) -> None" + assert doc(m.print_anyset) == "print_anyset(arg0: anyset) -> None" def test_frozenset(capture, doc): @@ -111,12 +111,8 @@ def test_frozenset(capture, doc): key: key3 """ ) - - assert m.anyset_size(frozenset()) == 0 assert m.anyset_size(s) == 3 - - assert m.anyset_empty(frozenset()) - assert not m.anyset_empty(frozenset({42})) + assert not m.anyset_empty(s) assert not m.anyset_contains(frozenset(), 42) assert m.anyset_contains(frozenset({42}), 42) @@ -338,6 +334,7 @@ def test_constructors(): list: range(3), dict: [("two", 2), ("one", 1), ("three", 3)], set: [4, 4, 5, 6, 6, 6], + frozenset: [4, 4, 5, 6, 6, 6], memoryview: b"abc", } inputs = {k.__name__: v for k, v in data.items()} From 05b6147ffb3d548ff79d70f985ef3231af5ef617 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 1 May 2022 15:01:45 +0000 Subject: [PATCH 6/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_pytypes.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 1f26625d14..8d296f655a 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -99,7 +99,8 @@ TEST_SUBMODULE(pytypes, m) { m.def("anyset_empty", [](const py::anyset &set) { return set.empty(); }); m.def("anyset_contains", [](const py::anyset &set, const py::object &key) { return set.contains(key); }); - m.def("anyset_contains", [](const py::anyset &set, const char *key) { return set.contains(key); }); + m.def("anyset_contains", + [](const py::anyset &set, const char *key) { return set.contains(key); }); m.def("set_add", [](py::set &set, const py::object &key) { set.add(key); }); m.def("set_clear", [](py::set &set) { set.clear(); }); From 0cf2caef08fb016a7a512a1249875724805496c9 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Mon, 2 May 2022 20:22:04 +0100 Subject: [PATCH 7/8] Add rationale to `pyobject_caster` default ctor --- include/pybind11/cast.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7d1e943dee..80fb23fe54 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -910,6 +910,8 @@ struct pyobject_caster { template ::value, int> = 0> pyobject_caster() : value() {} + // `type` may not be default constructible (e.g. frozenset, anyset). Initializing `value` + // to a nil handle is safe since it will only be accessed if `load` succeeds. template ::value, int> = 0> pyobject_caster() : value(reinterpret_steal(handle())) {} From c63ca46a5e20a035fe9f810549f8310e685e1be0 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Wed, 4 May 2022 22:32:50 +0100 Subject: [PATCH 8/8] Remove ineffectual protected: access control --- include/pybind11/pytypes.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index a79b0caadf..256a2441b1 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1785,10 +1785,8 @@ class kwargs : public dict { }; class anyset : public object { -protected: - PYBIND11_OBJECT(anyset, object, PyAnySet_Check) - public: + PYBIND11_OBJECT(anyset, object, PyAnySet_Check) size_t size() const { return static_cast(PySet_Size(m_ptr)); } bool empty() const { return size() == 0; } template