Adding ssize_t_cast to support passing size_t or ssize_t values where ssize_t is needed. (#3219)

* Trivial change to avoid (ssize_t) cast.

* Demo for safe_ssize_t idea.

* Removing safe_ssize_t.cpp (proof-of-concept code) to not upset the GHA Format workflow.

* Completing changes in pytypes.h

* New ssize_t_cast (better replacement for safe_ssize_t).

* clang-format-diff (no manual changes).

* bytes_ssize_t -Wnarrowing reproducer (see PR #2692).

* Backing out tuple(), list() ssize_t support, for compatibility with older compilers (to resolve link failures).

* Bug fix: missing `py::` for `py::ssize_t`

* Restoring tuple(), list() ssize_t support, but passing `size` by value, for compatibility with older compilers (to resolve link failures).

* Full test coverage of all functions with modified signatures.
This commit is contained in:
Ralf W. Grosse-Kunstleve 2021-08-28 16:40:46 -07:00 committed by GitHub
parent cb60ed49e4
commit 777352fcd1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 158 additions and 28 deletions

View File

@ -410,6 +410,12 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
using ssize_t = Py_ssize_t;
using size_t = std::size_t;
template <typename IntType>
inline ssize_t ssize_t_cast(const IntType &val) {
static_assert(sizeof(IntType) <= sizeof(ssize_t), "Implicit narrowing is not permitted.");
return static_cast<ssize_t>(val);
}
/// Approach used to cast a previously unknown C++ instance into a Python object
enum class return_value_policy : uint8_t {
/** This is the default return value policy, which falls back to the policy

View File

@ -661,15 +661,17 @@ struct generic_item {
struct sequence_item {
using key_type = size_t;
static object get(handle obj, size_t index) {
PyObject *result = PySequence_GetItem(obj.ptr(), static_cast<ssize_t>(index));
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static object get(handle obj, const IdxType &index) {
PyObject *result = PySequence_GetItem(obj.ptr(), ssize_t_cast(index));
if (!result) { throw error_already_set(); }
return reinterpret_steal<object>(result);
}
static void set(handle obj, size_t index, handle val) {
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static void set(handle obj, const IdxType &index, handle val) {
// PySequence_SetItem does not steal a reference to 'val'
if (PySequence_SetItem(obj.ptr(), static_cast<ssize_t>(index), val.ptr()) != 0) {
if (PySequence_SetItem(obj.ptr(), ssize_t_cast(index), val.ptr()) != 0) {
throw error_already_set();
}
}
@ -678,15 +680,17 @@ struct sequence_item {
struct list_item {
using key_type = size_t;
static object get(handle obj, size_t index) {
PyObject *result = PyList_GetItem(obj.ptr(), static_cast<ssize_t>(index));
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static object get(handle obj, const IdxType &index) {
PyObject *result = PyList_GetItem(obj.ptr(), ssize_t_cast(index));
if (!result) { throw error_already_set(); }
return reinterpret_borrow<object>(result);
}
static void set(handle obj, size_t index, handle val) {
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static void set(handle obj, const IdxType &index, handle val) {
// PyList_SetItem steals a reference to 'val'
if (PyList_SetItem(obj.ptr(), static_cast<ssize_t>(index), val.inc_ref().ptr()) != 0) {
if (PyList_SetItem(obj.ptr(), ssize_t_cast(index), val.inc_ref().ptr()) != 0) {
throw error_already_set();
}
}
@ -695,15 +699,17 @@ struct list_item {
struct tuple_item {
using key_type = size_t;
static object get(handle obj, size_t index) {
PyObject *result = PyTuple_GetItem(obj.ptr(), static_cast<ssize_t>(index));
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static object get(handle obj, const IdxType &index) {
PyObject *result = PyTuple_GetItem(obj.ptr(), ssize_t_cast(index));
if (!result) { throw error_already_set(); }
return reinterpret_borrow<object>(result);
}
static void set(handle obj, size_t index, handle val) {
template <typename IdxType, detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
static void set(handle obj, const IdxType &index, handle val) {
// PyTuple_SetItem steals a reference to 'val'
if (PyTuple_SetItem(obj.ptr(), static_cast<ssize_t>(index), val.inc_ref().ptr()) != 0) {
if (PyTuple_SetItem(obj.ptr(), ssize_t_cast(index), val.inc_ref().ptr()) != 0) {
throw error_already_set();
}
}
@ -1043,8 +1049,9 @@ class str : public object {
public:
PYBIND11_OBJECT_CVT(str, object, PYBIND11_STR_CHECK_FUN, raw_str)
str(const char *c, size_t n)
: object(PyUnicode_FromStringAndSize(c, (ssize_t) n), stolen_t{}) {
template <typename SzType, detail::enable_if_t<std::is_integral<SzType>::value, int> = 0>
str(const char *c, const SzType &n)
: object(PyUnicode_FromStringAndSize(c, ssize_t_cast(n)), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate string object!");
}
@ -1116,8 +1123,9 @@ public:
if (!m_ptr) pybind11_fail("Could not allocate bytes object!");
}
bytes(const char *c, size_t n)
: object(PYBIND11_BYTES_FROM_STRING_AND_SIZE(c, (ssize_t) n), stolen_t{}) {
template <typename SzType, detail::enable_if_t<std::is_integral<SzType>::value, int> = 0>
bytes(const char *c, const SzType &n)
: object(PYBIND11_BYTES_FROM_STRING_AND_SIZE(c, ssize_t_cast(n)), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate bytes object!");
}
@ -1160,7 +1168,7 @@ inline str::str(const bytes& b) {
ssize_t length = 0;
if (PYBIND11_BYTES_AS_STRING_AND_SIZE(b.ptr(), &buffer, &length))
pybind11_fail("Unable to extract bytes contents!");
auto obj = reinterpret_steal<object>(PyUnicode_FromStringAndSize(buffer, (ssize_t) length));
auto obj = reinterpret_steal<object>(PyUnicode_FromStringAndSize(buffer, length));
if (!obj)
pybind11_fail("Could not allocate string object!");
m_ptr = obj.release().ptr();
@ -1172,8 +1180,9 @@ class bytearray : public object {
public:
PYBIND11_OBJECT_CVT(bytearray, object, PyByteArray_Check, PyByteArray_FromObject)
bytearray(const char *c, size_t n)
: object(PyByteArray_FromStringAndSize(c, (ssize_t) n), stolen_t{}) {
template <typename SzType, detail::enable_if_t<std::is_integral<SzType>::value, int> = 0>
bytearray(const char *c, const SzType &n)
: object(PyByteArray_FromStringAndSize(c, ssize_t_cast(n)), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate bytearray object!");
}
@ -1398,7 +1407,10 @@ public:
class tuple : public object {
public:
PYBIND11_OBJECT_CVT(tuple, object, PyTuple_Check, PySequence_Tuple)
explicit tuple(size_t size = 0) : object(PyTuple_New((ssize_t) size), stolen_t{}) {
template <typename SzType = ssize_t,
detail::enable_if_t<std::is_integral<SzType>::value, int> = 0>
// Some compilers generate link errors when using `const SzType &` here:
explicit tuple(SzType size = 0) : object(PyTuple_New(ssize_t_cast(size)), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate tuple object!");
}
size_t size() const { return (size_t) PyTuple_Size(m_ptr); }
@ -1467,7 +1479,10 @@ public:
class list : public object {
public:
PYBIND11_OBJECT_CVT(list, object, PyList_Check, PySequence_List)
explicit list(size_t size = 0) : object(PyList_New((ssize_t) size), stolen_t{}) {
template <typename SzType = ssize_t,
detail::enable_if_t<std::is_integral<SzType>::value, int> = 0>
// Some compilers generate link errors when using `const SzType &` here:
explicit list(SzType size = 0) : object(PyList_New(ssize_t_cast(size)), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate list object!");
}
size_t size() const { return (size_t) PyList_Size(m_ptr); }
@ -1479,9 +1494,12 @@ public:
template <typename T> void append(T &&val) /* py-non-const */ {
PyList_Append(m_ptr, detail::object_or_cast(std::forward<T>(val)).ptr());
}
template <typename T> void insert(size_t index, T &&val) /* py-non-const */ {
PyList_Insert(m_ptr, static_cast<ssize_t>(index),
detail::object_or_cast(std::forward<T>(val)).ptr());
template <typename IdxType,
typename ValType,
detail::enable_if_t<std::is_integral<IdxType>::value, int> = 0>
void insert(const IdxType &index, ValType &&val) /* py-non-const */ {
PyList_Insert(
m_ptr, ssize_t_cast(index), detail::object_or_cast(std::forward<ValType>(val)).ptr());
}
};

View File

@ -168,12 +168,12 @@ public:
if (!std::is_lvalue_reference<T>::value)
policy = return_value_policy_override<Value>::policy(policy);
list l(src.size());
size_t index = 0;
ssize_t index = 0;
for (auto &&value : src) {
auto value_ = reinterpret_steal<object>(value_conv::cast(forward_like<T>(value), policy, parent));
if (!value_)
return handle();
PyList_SET_ITEM(l.ptr(), (ssize_t) index++, value_.release().ptr()); // steals a reference
PyList_SET_ITEM(l.ptr(), index++, value_.release().ptr()); // steals a reference
}
return l.release();
}
@ -225,12 +225,12 @@ public:
template <typename T>
static handle cast(T &&src, return_value_policy policy, handle parent) {
list l(src.size());
size_t index = 0;
ssize_t index = 0;
for (auto &&value : src) {
auto value_ = reinterpret_steal<object>(value_conv::cast(forward_like<T>(value), policy, parent));
if (!value_)
return handle();
PyList_SET_ITEM(l.ptr(), (ssize_t) index++, value_.release().ptr()); // steals a reference
PyList_SET_ITEM(l.ptr(), index++, value_.release().ptr()); // steals a reference
}
return l.release();
}

View File

@ -20,6 +20,11 @@ TEST_SUBMODULE(pytypes, m) {
// test_iterable
m.def("get_iterable", []{return py::iterable();});
// test_list
m.def("list_no_args", []() { return py::list{}; });
m.def("list_ssize_t", []() { return py::list{(py::ssize_t) 0}; });
m.def("list_size_t", []() { return py::list{(py::size_t) 0}; });
m.def("list_insert_ssize_t", [](py::list *l) { return l->insert((py::ssize_t) 1, 83); });
m.def("list_insert_size_t", [](py::list *l) { return l->insert((py::size_t) 3, 57); });
m.def("get_list", []() {
py::list list;
list.append("value");
@ -71,6 +76,9 @@ TEST_SUBMODULE(pytypes, m) {
[](const py::dict &dict, const char *val) { return dict.contains(val); });
// test_tuple
m.def("tuple_no_args", []() { return py::tuple{}; });
m.def("tuple_ssize_t", []() { return py::tuple{(py::ssize_t) 0}; });
m.def("tuple_size_t", []() { return py::tuple{(py::size_t) 0}; });
m.def("get_tuple", []() { return py::make_tuple(42, py::none(), "spam"); });
#if PY_VERSION_HEX >= 0x03030000
@ -84,6 +92,8 @@ TEST_SUBMODULE(pytypes, m) {
#endif
// test_str
m.def("str_from_char_ssize_t", []() { return py::str{"red", (py::ssize_t) 3}; });
m.def("str_from_char_size_t", []() { return py::str{"blue", (py::size_t) 4}; });
m.def("str_from_string", []() { return py::str(std::string("baz")); });
m.def("str_from_bytes", []() { return py::str(py::bytes("boo", 3)); });
m.def("str_from_object", [](const py::object& obj) { return py::str(obj); });
@ -100,10 +110,14 @@ TEST_SUBMODULE(pytypes, m) {
});
// test_bytes
m.def("bytes_from_char_ssize_t", []() { return py::bytes{"green", (py::ssize_t) 5}; });
m.def("bytes_from_char_size_t", []() { return py::bytes{"purple", (py::size_t) 6}; });
m.def("bytes_from_string", []() { return py::bytes(std::string("foo")); });
m.def("bytes_from_str", []() { return py::bytes(py::str("bar", 3)); });
// test bytearray
m.def("bytearray_from_char_ssize_t", []() { return py::bytearray{"$%", (py::ssize_t) 2}; });
m.def("bytearray_from_char_size_t", []() { return py::bytearray{"@$!", (py::size_t) 3}; });
m.def("bytearray_from_string", []() { return py::bytearray(std::string("foo")); });
m.def("bytearray_size", []() { return py::bytearray("foo").size(); });
@ -447,4 +461,57 @@ TEST_SUBMODULE(pytypes, m) {
m.def("weakref_from_object", [](const py::object &o) { return py::weakref(o); });
m.def("weakref_from_object_and_function",
[](py::object o, py::function f) { return py::weakref(std::move(o), std::move(f)); });
// Tests below this line are for pybind11 IMPLEMENTATION DETAILS:
m.def("sequence_item_get_ssize_t", [](const py::object &o) {
return py::detail::accessor_policies::sequence_item::get(o, (py::ssize_t) 1);
});
m.def("sequence_item_set_ssize_t", [](const py::object &o) {
auto s = py::str{"peppa", 5};
py::detail::accessor_policies::sequence_item::set(o, (py::ssize_t) 1, s);
});
m.def("sequence_item_get_size_t", [](const py::object &o) {
return py::detail::accessor_policies::sequence_item::get(o, (py::size_t) 2);
});
m.def("sequence_item_set_size_t", [](const py::object &o) {
auto s = py::str{"george", 6};
py::detail::accessor_policies::sequence_item::set(o, (py::size_t) 2, s);
});
m.def("list_item_get_ssize_t", [](const py::object &o) {
return py::detail::accessor_policies::list_item::get(o, (py::ssize_t) 3);
});
m.def("list_item_set_ssize_t", [](const py::object &o) {
auto s = py::str{"rebecca", 7};
py::detail::accessor_policies::list_item::set(o, (py::ssize_t) 3, s);
});
m.def("list_item_get_size_t", [](const py::object &o) {
return py::detail::accessor_policies::list_item::get(o, (py::size_t) 4);
});
m.def("list_item_set_size_t", [](const py::object &o) {
auto s = py::str{"richard", 7};
py::detail::accessor_policies::list_item::set(o, (py::size_t) 4, s);
});
m.def("tuple_item_get_ssize_t", [](const py::object &o) {
return py::detail::accessor_policies::tuple_item::get(o, (py::ssize_t) 5);
});
m.def("tuple_item_set_ssize_t", []() {
auto s0 = py::str{"emely", 5};
auto s1 = py::str{"edmond", 6};
auto o = py::tuple{2};
py::detail::accessor_policies::tuple_item::set(o, (py::ssize_t) 0, s0);
py::detail::accessor_policies::tuple_item::set(o, (py::ssize_t) 1, s1);
return o;
});
m.def("tuple_item_get_size_t", [](const py::object &o) {
return py::detail::accessor_policies::tuple_item::get(o, (py::size_t) 6);
});
m.def("tuple_item_set_size_t", []() {
auto s0 = py::str{"candy", 5};
auto s1 = py::str{"cat", 3};
auto o = py::tuple{2};
py::detail::accessor_policies::tuple_item::set(o, (py::size_t) 1, s1);
py::detail::accessor_policies::tuple_item::set(o, (py::size_t) 0, s0);
return o;
});
}

View File

@ -23,6 +23,15 @@ def test_iterable(doc):
def test_list(capture, doc):
assert m.list_no_args() == []
assert m.list_ssize_t() == []
assert m.list_size_t() == []
lins = [1, 2]
m.list_insert_ssize_t(lins)
assert lins == [1, 83, 2]
m.list_insert_size_t(lins)
assert lins == [1, 83, 2, 57]
with capture:
lst = m.get_list()
assert lst == ["inserted-0", "overwritten", "inserted-2"]
@ -100,6 +109,9 @@ def test_dict(capture, doc):
def test_tuple():
assert m.tuple_no_args() == ()
assert m.tuple_ssize_t() == ()
assert m.tuple_size_t() == ()
assert m.get_tuple() == (42, None, "spam")
@ -113,6 +125,8 @@ def test_simple_namespace():
def test_str(doc):
assert m.str_from_char_ssize_t().encode().decode() == "red"
assert m.str_from_char_size_t().encode().decode() == "blue"
assert m.str_from_string().encode().decode() == "baz"
assert m.str_from_bytes().encode().decode() == "boo"
@ -157,6 +171,8 @@ def test_str(doc):
def test_bytes(doc):
assert m.bytes_from_char_ssize_t().decode() == "green"
assert m.bytes_from_char_size_t().decode() == "purple"
assert m.bytes_from_string().decode() == "foo"
assert m.bytes_from_str().decode() == "bar"
@ -166,6 +182,8 @@ def test_bytes(doc):
def test_bytearray(doc):
assert m.bytearray_from_char_ssize_t().decode() == "$%"
assert m.bytearray_from_char_size_t().decode() == "@$!"
assert m.bytearray_from_string().decode() == "foo"
assert m.bytearray_size() == len("foo")
@ -603,3 +621,24 @@ def test_weakref(create_weakref, create_weakref_with_callback):
del obj
pytest.gc_collect()
assert callback.called
def test_implementation_details():
lst = [39, 43, 92, 49, 22, 29, 93, 98, 26, 57, 8]
tup = tuple(lst)
assert m.sequence_item_get_ssize_t(lst) == 43
assert m.sequence_item_set_ssize_t(lst) is None
assert lst[1] == "peppa"
assert m.sequence_item_get_size_t(lst) == 92
assert m.sequence_item_set_size_t(lst) is None
assert lst[2] == "george"
assert m.list_item_get_ssize_t(lst) == 49
assert m.list_item_set_ssize_t(lst) is None
assert lst[3] == "rebecca"
assert m.list_item_get_size_t(lst) == 22
assert m.list_item_set_size_t(lst) is None
assert lst[4] == "richard"
assert m.tuple_item_get_ssize_t(tup) == 29
assert m.tuple_item_set_ssize_t() == ("emely", "edmond")
assert m.tuple_item_get_size_t(tup) == 93
assert m.tuple_item_set_size_t() == ("candy", "cat")