From d7ac59ea61dfb0462cee03ed3ba42e8f9ab8b9cc Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Tue, 24 May 2016 14:50:19 -0600 Subject: [PATCH 01/18] Add smart pointer classes for managing pyobject pointers. In particular, there are 4 possible sets of smart pointer semantics defined here corresponding to the cases where a PyObject pointer does or does not own a reference and where a pointer is or is not allowed to be null valued. --- dynd/include/utility_functions.hpp | 322 +++++++++++++++++++++++------ 1 file changed, 255 insertions(+), 67 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index a6596901..1ac44543 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -22,86 +22,280 @@ struct callable_type_data; namespace pydynd { -/** - * A container class for managing the local lifetime of - * PyObject *. - * - * Throws an exception if the object passed into the constructor - * is NULL. - */ -class pyobject_ownref { - PyObject *m_obj; +template +inline void incref(PyObject *) noexcept; + +template <> +inline void incref(PyObject *obj) noexcept +{ + Py_XINCREF(obj); +} + +template <> +inline void incref(PyObject *obj) noexcept +{ + Py_INCREF(obj); +} + +template +inline void decref(PyObject *) noexcept; + +template <> +inline void decref(PyObject *obj) noexcept +{ + Py_XDECREF(obj); +} + +template <> +inline void decref(PyObject *obj) noexcept +{ + Py_DECREF(obj); +} + +template +inline void incref_if_owned(PyObject *obj) noexcept; + +template <> +inline void incref_if_owned(PyObject *obj) noexcept +{ + Py_INCREF(obj); +} + +template <> +inline void incref_if_owned(PyObject *obj) noexcept +{ + Py_XINCREF(obj); +} + +template <> +inline void incref_if_owned(PyObject *obj) noexcept +{ +} + +template <> +inline void incref_if_owned(PyObject *obj) noexcept +{ +} + +template +inline void decref_if_owned(PyObject *obj) noexcept; + +template <> +inline void decref_if_owned(PyObject *obj) noexcept +{ + Py_DECREF(obj); +} + +template <> +inline void decref_if_owned(PyObject *obj) noexcept +{ + Py_XDECREF(obj); +} + +template <> +inline void decref_if_owned(PyObject *obj) noexcept +{ +} + +template <> +inline void decref_if_owned(PyObject *obj) noexcept +{ +} - // Non-copyable - pyobject_ownref(const pyobject_ownref &); - pyobject_ownref &operator=(const pyobject_ownref &); +template +class py_ref { + PyObject *o; public: - inline pyobject_ownref() : m_obj(NULL) {} - inline explicit pyobject_ownref(PyObject *obj) : m_obj(obj) + // Explicit specializations for default constructor + // are defined after the class declaration. + // If the class is allowed to be null, it default-initializes to null. + // If the class is not allowed to be null, it default-initializes to None. + py_ref() noexcept; + + // First define an accessor to get the PyObject pointer from + // the wrapper class. + PyObject *get() noexcept { return o; } + + // All copy and move constructors are defined as noexcept. + // If the conversion or move operation would move from a type + // that can be null to one that can not, the corresponding + // constructor is declared explicit. + + /* If: + * this type allows null, + * Or: + * this type doesn't allow null + * and the input type doesn't allow null, + * Then: + * Allow implicit conversions from the other py_ref type to this type. + */ + template * = nullptr> + py_ref(const py_ref &other) noexcept { - if (obj == NULL) { - throw std::runtime_error("propagating a Python exception..."); - } + o = other.o; + incref_if_owned(o); } - inline pyobject_ownref(PyObject *obj, bool inc_ref) : m_obj(obj) + /* If: + * this type doesn't allow null, + * and the input type does, + * Then: + * Require that conversions from the other py_ref type to this type be explcit. + */ + template * = nullptr> + explicit py_ref(const py_ref &other) noexcept { - if (obj == NULL) { - throw std::runtime_error("propagating a Python exception..."); - } - if (inc_ref) { - Py_INCREF(m_obj); - } + o = other.o; + incref_if_owned(o); } - inline ~pyobject_ownref() { Py_XDECREF(m_obj); } - - inline PyObject **obj_addr() { return &m_obj; } + // Move constructors are really only useful for moving + // between types that own their references. + // Only define them for those cases. - /** - * Resets the reference owned by this object to the one provided. - * This steals a reference to the input parameter, 'obj'. - * - * \param obj The reference to replace the current one in this object. + /* If: + * both this type and the input type own their reference, + * and we are not moving from a type that allows null values to one that does not, + * Then: + * a move operation can be implicitly performed. */ - inline void reset(PyObject *obj) + template * = nullptr> + py_ref(py_ref &&other) noexcept { - if (obj == NULL) { - throw std::runtime_error("propagating a Python exception..."); - } - Py_XDECREF(m_obj); - m_obj = obj; + o = other.o; } - /** - * Clears the owned reference to NULL. + /* If: + * both this type and the input type own their reference, + * and we are moving from a type that allows null values to one that does not, + * Then: + * only an explicit move operation can be performed. */ - inline void clear() + template * = nullptr> + explicit py_ref(py_ref &&other) noexcept { - Py_XDECREF(m_obj); - m_obj = NULL; + o = other.o; } - /** Returns a borrowed reference. */ - inline PyObject *get() const { return m_obj; } - - /** Returns a borrowed reference. */ - inline operator PyObject *() { return m_obj; } + /* When constructing from a PyObject*, the boolean value `consume_ref` must + * be passed to the constructor so it is clear whether or not to + * increment, decrement, or do nothing the reference when storing it in the + * desired smart pointer type. If you do not intend for the smart + * pointer to capture the reference that you own, you should + * specify `consume_ref` as false regardless of whether or not you + * own the reference represented in the PyObject* you pass in. + */ + explicit py_ref(PyObject *obj, bool consume_ref) noexcept + { + o = obj; + if (consume_ref) { + decref_if_owned(o); + } + else { + incref_if_owned(o); + } + } - /** - * Returns the reference owned by this object, - * use it like "return obj.release()". After the - * call, this object contains NULL. + ~py_ref() { decref_if_owned(o); } + + // For assignment operators, only allow assignment + // in cases where implicit conversions are also allowed. + // This forces explicit handling of cases that could + // need to have an exception raised. + // For that reason, these are all marked as noexcept. + // Assignment never comsumes a reference. + + /* If: + * this type allows null, + * Or: + * this type doesn't allow null + * and the input type doesn't allow null, + * Then: + * Allow assignment from the other py_ref type to this type. */ - inline PyObject *release() + template * = nullptr> + py_ref operator=(const py_ref &other) noexcept { - PyObject *result = m_obj; - m_obj = NULL; - return result; + decref_if_owned(o); + o = other.o; + incref_if_owned(o); + } + + // Check if the wrapped pointer is null. + // Always return true if it is not null by definition. + bool is_null() noexcept + { + // This function will be a no-op returning true + // when not_null is false. + if (not_null || (o != nullptr)) { + return false; + } + return true; } + + // A debug version of is_null with a purely dynamic check. + bool is_null_dbg() noexcept { return o != nullptr; } }; +// Default constructors for various cases. +template <> +inline py_ref::py_ref() noexcept +{ + o = Py_None; + incref(o); +} + +template <> +inline py_ref::py_ref() noexcept +{ + o = nullptr; +} + +template <> +inline py_ref::py_ref() noexcept +{ + o = Py_None; +} + +template <> +inline py_ref::py_ref() noexcept +{ + o = nullptr; +} + +// Convenience aliases for the templated smart pointer classes. +// All the template arguments to py_ref have defaults, +// so py_ref can already be used for pointers that +// own their reference and cannot be null. +// The following aliases fill in the other cases. + +using py_ref_with_null = py_ref; + +using py_borref = py_ref; + +using py_borref_with_null = py_ref; + +// To help with the transition to the new classes. +using pyobject_ownref = py_ref; + +/* Check if a wrapped pointer is null. + * If it is not, return the pointer + * wrapped in the corresponding not_null wrapper type. + * If it is, raise an exception. + * This can be used to forward exceptions from Python. + */ +template +py_ref check_null(py_ref &o) +{ + if (o.is_null()) { + throw std::runtime_error("Unexpected null pointer."); + } + return reinterpret_cast>(o); +} + class PyGILState_RAII { PyGILState_STATE m_gstate; @@ -149,8 +343,7 @@ inline intptr_t pyobject_as_index(PyObject *index) #endif } else { - throw std::runtime_error( - "Value returned from PyNumber_Index is not an int or long"); + throw std::runtime_error("Value returned from PyNumber_Index is not an int or long"); } if (result == -1 && PyErr_Occurred()) { throw std::exception(); @@ -170,8 +363,7 @@ inline int pyobject_as_int_index(PyObject *index) throw std::exception(); } if (((unsigned long)result & 0xffffffffu) != (unsigned long)result) { - throw std::overflow_error( - "overflow converting Python integer to 32-bit int"); + throw std::overflow_error("overflow converting Python integer to 32-bit int"); } return (int)result; } @@ -248,8 +440,7 @@ inline std::string pyobject_repr(PyObject *obj) return pystring_as_string(src_repr.get()); } -inline void pyobject_as_vector_string(PyObject *list_string, - std::vector &vector_string) +inline void pyobject_as_vector_string(PyObject *list_string, std::vector &vector_string) { Py_ssize_t size = PySequence_Size(list_string); vector_string.resize(size); @@ -259,9 +450,7 @@ inline void pyobject_as_vector_string(PyObject *list_string, } } -inline void pyobject_as_vector_intp(PyObject *list_index, - std::vector &vector_intp, - bool allow_int) +inline void pyobject_as_vector_intp(PyObject *list_index, std::vector &vector_intp, bool allow_int) { if (allow_int) { // If permitted, convert an int into a size-1 list @@ -389,8 +578,7 @@ inline void mark_axis(PyObject *int_axis, int ndim, dynd::bool1 *reduce_axes) * * Returns the number of axes which were set. */ -inline int pyarg_axis_argument(PyObject *axis, int ndim, - dynd::bool1 *reduce_axes) +inline int pyarg_axis_argument(PyObject *axis, int ndim, dynd::bool1 *reduce_axes) { int axis_count = 0; From 844d532121ef28f82154f7c95f17db63fe159d36 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Tue, 24 May 2016 17:27:20 -0600 Subject: [PATCH 02/18] Update dynd-python codebase to use new py_ref class. --- .../assign_to_pyarrayobject_callable.hpp | 219 +++++++++--------- .../callables/assign_to_pyobject_callable.hpp | 12 +- .../include/kernels/apply_pyobject_kernel.hpp | 38 +-- .../kernels/assign_from_pyobject_kernel.hpp | 34 +-- .../kernels/assign_to_pyobject_kernel.hpp | 32 +-- dynd/include/numpy_type_interop.hpp | 7 +- dynd/include/type_functions.hpp | 2 +- dynd/include/utility_functions.hpp | 79 ++++--- dynd/src/array_as_numpy.cpp | 183 ++++++++------- dynd/src/array_from_py.cpp | 6 +- dynd/src/numpy_interop.cpp | 4 +- dynd/src/numpy_type_interop.cpp | 98 ++++---- dynd/src/type_deduction.cpp | 20 +- 13 files changed, 378 insertions(+), 356 deletions(-) diff --git a/dynd/include/callables/assign_to_pyarrayobject_callable.hpp b/dynd/include/callables/assign_to_pyarrayobject_callable.hpp index b5d841ba..7e9329a1 100644 --- a/dynd/include/callables/assign_to_pyarrayobject_callable.hpp +++ b/dynd/include/callables/assign_to_pyarrayobject_callable.hpp @@ -1,7 +1,7 @@ #pragma once -#include #include "kernels/assign_to_pyarrayobject_kernel.hpp" +#include /** * This sets up a ckernel to copy from a dynd array @@ -22,127 +22,128 @@ class assign_to_pyarrayobject_callable : public dynd::nd::base_callable { return dst_tp; } -/* - void instantiate(char *data, dynd::nd::kernel_builder *ckb, const dynd::ndt::type &dst_tp, const char *dst_arrmeta, - intptr_t nsrc, const dynd::ndt::type *src_tp, const char *const *src_arrmeta, - dynd::kernel_request_t kernreq, intptr_t nkwd, const dynd::nd::array *kwds, - const std::map &tp_vars) - { - PyObject *dst_obj = *reinterpret_cast(dst_arrmeta); - uintptr_t dst_alignment = reinterpret_cast(dst_arrmeta)[1]; + /* + void instantiate(char *data, dynd::nd::kernel_builder *ckb, const dynd::ndt::type &dst_tp, const char *dst_arrmeta, + intptr_t nsrc, const dynd::ndt::type *src_tp, const char *const *src_arrmeta, + dynd::kernel_request_t kernreq, intptr_t nkwd, const dynd::nd::array *kwds, + const std::map &tp_vars) + { + PyObject *dst_obj = *reinterpret_cast(dst_arrmeta); + uintptr_t dst_alignment = reinterpret_cast(dst_arrmeta)[1]; - PyArray_Descr *dtype = reinterpret_cast(dst_obj); - - // If there is no object type in the numpy type, get the dynd equivalent - // type and use it to do the copying - if (!PyDataType_FLAGCHK(dtype, NPY_ITEM_HASOBJECT)) { - dynd::ndt::type dst_view_tp = pydynd::_type_from_numpy_dtype(dtype, dst_alignment); - nd::array error_mode = assign_error_fractional; - nd::assign->instantiate(node, NULL, ckb, dst_view_tp, NULL, 1, src_tp, src_arrmeta, kernreq, 1, &error_mode, - std::map()); - return; - } + PyArray_Descr *dtype = reinterpret_cast(dst_obj); - if (PyDataType_ISOBJECT(dtype)) { - dynd::nd::assign->instantiate(node, NULL, ckb, dynd::ndt::make_type(), NULL, nsrc, src_tp, - src_arrmeta, kernreq, 0, NULL, tp_vars); - return; - } - - if (PyDataType_HASFIELDS(dtype)) { - if (src_tp[0].get_id() != dynd::struct_id && src_tp[0].get_id() != dynd::tuple_id) { - std::stringstream ss; - pydynd::pyobject_ownref dtype_str(PyObject_Str((PyObject *)dtype)); - ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " - << pydynd::pystring_as_string(dtype_str.get()); - throw std::invalid_argument(ss.str()); + // If there is no object type in the numpy type, get the dynd equivalent + // type and use it to do the copying + if (!PyDataType_FLAGCHK(dtype, NPY_ITEM_HASOBJECT)) { + dynd::ndt::type dst_view_tp = pydynd::_type_from_numpy_dtype(dtype, dst_alignment); + nd::array error_mode = assign_error_fractional; + nd::assign->instantiate(node, NULL, ckb, dst_view_tp, NULL, 1, src_tp, src_arrmeta, kernreq, 1, &error_mode, + std::map()); + return; } - // Get the fields out of the numpy dtype - std::vector field_dtypes_orig; - std::vector field_names_orig; - std::vector field_offsets_orig; - pydynd::extract_fields_from_numpy_struct(dtype, field_dtypes_orig, field_names_orig, field_offsets_orig); - intptr_t field_count = field_dtypes_orig.size(); - if (field_count != src_tp[0].extended()->get_field_count()) { - std::stringstream ss; - pydynd::pyobject_ownref dtype_str(PyObject_Str((PyObject *)dtype)); - ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " - << pydynd::pystring_as_string(dtype_str.get()); - throw std::invalid_argument(ss.str()); + if (PyDataType_ISOBJECT(dtype)) { + dynd::nd::assign->instantiate(node, NULL, ckb, dynd::ndt::make_type(), NULL, nsrc, src_tp, + src_arrmeta, kernreq, 0, NULL, tp_vars); + return; } - // Permute the numpy fields to match with the dynd fields - std::vector field_dtypes; - std::vector field_offsets; - if (src_tp[0].get_id() == dynd::struct_id) { - field_dtypes.resize(field_count); - field_offsets.resize(field_count); - for (intptr_t i = 0; i < field_count; ++i) { - intptr_t src_i = src_tp[0].extended()->get_field_index(field_names_orig[i]); - if (src_i >= 0) { - field_dtypes[src_i] = field_dtypes_orig[i]; - field_offsets[src_i] = field_offsets_orig[i]; - } - else { - std::stringstream ss; - pydynd::pyobject_ownref dtype_str(PyObject_Str((PyObject *)dtype)); - ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " - << pydynd::pystring_as_string(dtype_str.get()); - throw std::invalid_argument(ss.str()); + if (PyDataType_HASFIELDS(dtype)) { + if (src_tp[0].get_id() != dynd::struct_id && src_tp[0].get_id() != dynd::tuple_id) { + std::stringstream ss; + pydynd::py_ref dtype_str = capture_if_not_null(PyObject_Str((PyObject *)dtype)); + ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " + << pydynd::pystring_as_string(dtype_str.get()); + throw std::invalid_argument(ss.str()); + } + + // Get the fields out of the numpy dtype + std::vector field_dtypes_orig; + std::vector field_names_orig; + std::vector field_offsets_orig; + pydynd::extract_fields_from_numpy_struct(dtype, field_dtypes_orig, field_names_orig, field_offsets_orig); + intptr_t field_count = field_dtypes_orig.size(); + if (field_count != src_tp[0].extended()->get_field_count()) { + std::stringstream ss; + pydynd::py_ref dtype_str = capture_if_not_null(PyObject_Str((PyObject *)dtype)); + ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " + << pydynd::pystring_as_string(dtype_str.get()); + throw std::invalid_argument(ss.str()); + } + + // Permute the numpy fields to match with the dynd fields + std::vector field_dtypes; + std::vector field_offsets; + if (src_tp[0].get_id() == dynd::struct_id) { + field_dtypes.resize(field_count); + field_offsets.resize(field_count); + for (intptr_t i = 0; i < field_count; ++i) { + intptr_t src_i = src_tp[0].extended()->get_field_index(field_names_orig[i]); + if (src_i >= 0) { + field_dtypes[src_i] = field_dtypes_orig[i]; + field_offsets[src_i] = field_offsets_orig[i]; + } + else { + std::stringstream ss; + pydynd::py_ref dtype_str = capture_if_not_null(PyObject_Str((PyObject *)dtype)); + ss << "Cannot assign from source dynd type " << src_tp[0] << " to numpy type " + << pydynd::pystring_as_string(dtype_str.get()); + throw std::invalid_argument(ss.str()); + } } } - } - else { - // In the tuple case, use position instead of name - field_dtypes.swap(field_dtypes_orig); - field_offsets.swap(field_offsets_orig); - } + else { + // In the tuple case, use position instead of name + field_dtypes.swap(field_dtypes_orig); + field_offsets.swap(field_offsets_orig); + } - std::vector dst_fields_tp(field_count, dynd::ndt::make_type()); - std::vector dst_arrmeta_values(field_count); - std::vector dst_fields_arrmeta(field_count); - for (intptr_t i = 0; i < field_count; ++i) { - dst_arrmeta_values[i].dst_dtype = field_dtypes[i]; - dst_arrmeta_values[i].dst_alignment = dst_alignment | field_offsets[i]; - dst_fields_arrmeta[i] = reinterpret_cast(&dst_arrmeta_values[i]); - } + std::vector dst_fields_tp(field_count, dynd::ndt::make_type()); + std::vector dst_arrmeta_values(field_count); + std::vector dst_fields_arrmeta(field_count); + for (intptr_t i = 0; i < field_count; ++i) { + dst_arrmeta_values[i].dst_dtype = field_dtypes[i]; + dst_arrmeta_values[i].dst_alignment = dst_alignment | field_offsets[i]; + dst_fields_arrmeta[i] = reinterpret_cast(&dst_arrmeta_values[i]); + } - const uintptr_t *src_arrmeta_offsets = src_tp[0].extended()->get_arrmeta_offsets_raw(); - dynd::shortvector src_fields_arrmeta(field_count); - for (intptr_t i = 0; i != field_count; ++i) { - src_fields_arrmeta[i] = src_arrmeta[0] + src_arrmeta_offsets[i]; - } + const uintptr_t *src_arrmeta_offsets = src_tp[0].extended()->get_arrmeta_offsets_raw(); + dynd::shortvector src_fields_arrmeta(field_count); + for (intptr_t i = 0; i != field_count; ++i) { + src_fields_arrmeta[i] = src_arrmeta[0] + src_arrmeta_offsets[i]; + } - // Todo: Remove this - dynd::nd::callable af = dynd::nd::make_callable(); + // Todo: Remove this + dynd::nd::callable af = dynd::nd::make_callable(); - const std::vector &src_field_tp = src_tp[0].extended()->get_field_types(); - const uintptr_t *src_data_offsets = src_tp[0].extended()->get_data_offsets(src_arrmeta[0]); + const std::vector &src_field_tp = src_tp[0].extended()->get_field_types(); + const uintptr_t *src_data_offsets = + src_tp[0].extended()->get_data_offsets(src_arrmeta[0]); - intptr_t self_offset = ckb->size(); - ckb->emplace_back(kernreq); - nd::tuple_unary_op_ck *self = ckb->get_at(self_offset); - self->m_fields.resize(field_count); - for (intptr_t i = 0; i < field_count; ++i) { - self = ckb->get_at(self_offset); - nd::tuple_unary_op_item &field = self->m_fields[i]; - field.child_kernel_offset = ckb->size() - self_offset; - field.dst_data_offset = field_offsets[i]; - field.src_data_offset = src_data_offsets[i]; - nd::array error_mode = ndt::traits::na(); - af->instantiate(node, NULL, ckb, dst_fields_tp[i], dst_fields_arrmeta[i], 1, &src_field_tp[i], - &src_fields_arrmeta[i], kernel_request_single, 1, &error_mode, - std::map()); + intptr_t self_offset = ckb->size(); + ckb->emplace_back(kernreq); + nd::tuple_unary_op_ck *self = ckb->get_at(self_offset); + self->m_fields.resize(field_count); + for (intptr_t i = 0; i < field_count; ++i) { + self = ckb->get_at(self_offset); + nd::tuple_unary_op_item &field = self->m_fields[i]; + field.child_kernel_offset = ckb->size() - self_offset; + field.dst_data_offset = field_offsets[i]; + field.src_data_offset = src_data_offsets[i]; + nd::array error_mode = ndt::traits::na(); + af->instantiate(node, NULL, ckb, dst_fields_tp[i], dst_fields_arrmeta[i], 1, &src_field_tp[i], + &src_fields_arrmeta[i], kernel_request_single, 1, &error_mode, + std::map()); + } + return; + } + else { + std::stringstream ss; + ss << "TODO: implement assign from source dynd type " << src_tp[0] << " to numpy type " + << pydynd::pyobject_repr((PyObject *)dtype); + throw std::invalid_argument(ss.str()); } - return; - } - else { - std::stringstream ss; - ss << "TODO: implement assign from source dynd type " << src_tp[0] << " to numpy type " - << pydynd::pyobject_repr((PyObject *)dtype); - throw std::invalid_argument(ss.str()); } - } -*/ + */ }; diff --git a/dynd/include/callables/assign_to_pyobject_callable.hpp b/dynd/include/callables/assign_to_pyobject_callable.hpp index 72e8bc6e..c0ebe26f 100644 --- a/dynd/include/callables/assign_to_pyobject_callable.hpp +++ b/dynd/include/callables/assign_to_pyobject_callable.hpp @@ -289,11 +289,12 @@ namespace nd { ckb_offset = kb.size(); self_ck->m_src_tp = src0_tp; self_ck->m_src_arrmeta = src_arrmeta[0]; - self_ck->m_field_names.reset(PyTuple_New(field_count)); + self_ck->m_field_names = capture_if_not_null(PyTuple_New(field_count)); for (intptr_t i = 0; i < field_count; ++i) { const dynd::string &rawname = src0_tp.extended()->get_field_name(i); - pydynd::pyobject_ownref name(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); - PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.release()); + pydynd::py_ref name = + capture_if_not_null(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); + PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.get()); } self_ck->m_copy_el_offsets.resize(field_count); @@ -333,10 +334,11 @@ namespace nd { intptr_t field_count = src_tp[0].extended()->get_field_count(); const dynd::ndt::type *field_types = src_tp[0].extended()->get_field_types_raw(); const uintptr_t *arrmeta_offsets = src_tp[0].extended()->get_arrmeta_offsets_raw(); - self_ck->m_field_names.reset(PyTuple_New(field_count)); + self_ck->m_field_names = capture_if_not_null(PyTuple_New(field_count)); for (intptr_t i = 0; i < field_count; ++i) { const dynd::string &rawname = src_tp[0].extended()->get_field_name(i); - pydynd::pyobject_ownref name(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); + pydynd::py_ref name = + capture_if_not_null(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.release()); } self_ck->m_copy_el_offsets.resize(field_count); diff --git a/dynd/include/kernels/apply_pyobject_kernel.hpp b/dynd/include/kernels/apply_pyobject_kernel.hpp index d3332a33..f37fc795 100644 --- a/dynd/include/kernels/apply_pyobject_kernel.hpp +++ b/dynd/include/kernels/apply_pyobject_kernel.hpp @@ -37,7 +37,7 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernelget_use_count() != 1) { std::stringstream ss; ss << "Python callback function "; - pydynd::pyobject_ownref pyfunc_repr(PyObject_Repr(m_pyfunc)); + pydynd::py_ref pyfunc_repr = pydynd::capture_if_not_null(PyObject_Repr(m_pyfunc)); ss << pydynd::pystring_as_string(pyfunc_repr.get()); ss << ", called by dynd, held a reference to parameter "; ss << (i + 1) << " which contained temporary memory."; @@ -70,7 +70,7 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernelget_return_type(); const std::vector &src_tp = fpt->get_argument_types(); // First set up the parameters in a tuple - pydynd::pyobject_ownref args(PyTuple_New(nsrc)); + pydynd::py_ref args = pydynd::capture_if_not_null(PyTuple_New(nsrc)); for (intptr_t i = 0; i != nsrc; ++i) { dynd::ndt::type tp = src_tp[i]; dynd::nd::array n = dynd::nd::make_array(tp, const_cast(src[i]), dynd::nd::read_access_flag); @@ -80,12 +80,15 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernel(&child_obj); - get_child()->single(dst, &child_src); - res.clear(); + PyObject *child_obj; + char *child_src; + { // This scope exists to limit the lifetime of py_ref res. + pydynd::py_ref res = pydynd::capture_if_not_null(PyObject_Call(m_pyfunc, args.get(), NULL)); + // Copy the result into the destination memory + child_obj = res.get(); + child_src = reinterpret_cast(&child_obj); + get_child()->single(dst, &child_src); + } // Validate that the call didn't hang onto the ephemeral data // pointers we used. This is done after the dst assignment, because // the function result may have contained a reference to an argument. @@ -99,7 +102,7 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernelget_return_type(); const std::vector &src_tp = fpt->get_argument_types(); // First set up the parameters in a tuple - pydynd::pyobject_ownref args(PyTuple_New(nsrc)); + pydynd::py_ref args = pydynd::capture_if_not_null(PyTuple_New(nsrc)); for (intptr_t i = 0; i != nsrc; ++i) { dynd::ndt::type tp = src_tp[i]; dynd::nd::array n = dynd::nd::make_array(tp, const_cast(src[i]), dynd::nd::read_access_flag); @@ -111,12 +114,15 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernel(&child_obj); - get_child()->single(dst, &child_src); - res.clear(); + PyObject *child_obj; + char *child_src; + { // Scope to hold lifetime of py_ref res. + pydynd::py_ref res = pydynd::capture_if_not_null(PyObject_Call(m_pyfunc, args.get(), NULL)); + // Copy the result into the destination memory + child_obj = res.get(); + child_src = reinterpret_cast(&child_obj); + get_child()->single(dst, &child_src); + } // Validate that the call didn't hang onto the ephemeral data // pointers we used. This is done after the dst assignment, because // the function result may have contained a reference to an argument. @@ -125,7 +131,7 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kernelset_data(n->get_data() + src_stride[i]); + // n->set_data(n->get_data() + src_stride[i]); } } } diff --git a/dynd/include/kernels/assign_from_pyobject_kernel.hpp b/dynd/include/kernels/assign_from_pyobject_kernel.hpp index 68d3be0c..3739d13d 100644 --- a/dynd/include/kernels/assign_from_pyobject_kernel.hpp +++ b/dynd/include/kernels/assign_from_pyobject_kernel.hpp @@ -126,13 +126,13 @@ void pyint_to_int(dynd::int128 *out, PyObject *obj) } #endif uint64_t lo = PyLong_AsUnsignedLongLongMask(obj); - pydynd::pyobject_ownref sixtyfour(PyLong_FromLong(64)); - pydynd::pyobject_ownref value_shr1(PyNumber_Rshift(obj, sixtyfour.get())); + pydynd::py_ref sixtyfour = pydynd::capture_if_not_null(PyLong_FromLong(64)); + pydynd::py_ref value_shr1 = pydynd::capture_if_not_null(PyNumber_Rshift(obj, sixtyfour.get())); uint64_t hi = PyLong_AsUnsignedLongLongMask(value_shr1.get()); dynd::int128 result(hi, lo); // Shift right another 64 bits, and check that nothing is remaining - pydynd::pyobject_ownref value_shr2(PyNumber_Rshift(value_shr1.get(), sixtyfour.get())); + pydynd::py_ref value_shr2 = pydynd::capture_if_not_null(PyNumber_Rshift(value_shr1.get(), sixtyfour.get())); long remaining = PyLong_AsLong(value_shr2.get()); if ((remaining != 0 || (remaining == 0 && result.is_negative())) && (remaining != -1 || PyErr_Occurred() || (remaining == -1 && !result.is_negative()))) { @@ -210,13 +210,13 @@ void pyint_to_int(dynd::uint128 *out, PyObject *obj) } #endif uint64_t lo = PyLong_AsUnsignedLongLongMask(obj); - pydynd::pyobject_ownref sixtyfour(PyLong_FromLong(64)); - pydynd::pyobject_ownref value_shr1(PyNumber_Rshift(obj, sixtyfour.get())); + pydynd::py_ref sixtyfour = pydynd::capture_if_not_null(PyLong_FromLong(64)); + pydynd::py_ref value_shr1 = pydynd::capture_if_not_null(PyNumber_Rshift(obj, sixtyfour.get())); uint64_t hi = PyLong_AsUnsignedLongLongMask(value_shr1.get()); dynd::uint128 result(hi, lo); // Shift right another 64 bits, and check that nothing is remaining - pydynd::pyobject_ownref value_shr2(PyNumber_Rshift(value_shr1.get(), sixtyfour.get())); + pydynd::py_ref value_shr2 = pydynd::capture_if_not_null(PyNumber_Rshift(value_shr1.get(), sixtyfour.get())); long remaining = PyLong_AsLong(value_shr2.get()); if (remaining != 0) { throw std::overflow_error("int is too big to fit in an uint128"); @@ -381,7 +381,7 @@ struct assign_from_pyobject_kernel if (PyUnicode_Check(src_obj)) { // Go through UTF8 (was accessing the cpython unicode values directly // before, but on Python 3.3 OS X it didn't work correctly.) - pydynd::pyobject_ownref utf8(PyUnicode_AsUTF8String(src_obj)); + pydynd::py_ref utf8 = pydynd::capture_if_not_null(PyUnicode_AsUTF8String(src_obj)); char *s = NULL; Py_ssize_t len = 0; if (PyBytes_AsStringAndSize(utf8.get(), &s, &len) < 0) { @@ -465,7 +465,7 @@ struct assign_from_pyobject_kernel } else if (dst_tp.get_base_id() != dynd::string_kind_id && PyUnicode_Check(src_obj)) { // Copy from the string - pydynd::pyobject_ownref utf8(PyUnicode_AsUTF8String(src_obj)); + pydynd::py_ref utf8 = pydynd::capture_if_not_null(PyUnicode_AsUTF8String(src_obj)); char *s = NULL; Py_ssize_t len = 0; if (PyBytes_AsStringAndSize(utf8.get(), &s, &len) < 0) { @@ -539,7 +539,7 @@ struct assign_from_pyobject_kernel const uintptr_t *field_offsets = reinterpret_cast(m_dst_arrmeta); // Get the input as an array of PyObject * - pydynd::pyobject_ownref src_fast; + pydynd::py_ref src_fast; char *child_src; intptr_t child_stride = sizeof(PyObject *); intptr_t src_dim_size; @@ -548,7 +548,7 @@ struct assign_from_pyobject_kernel src_dim_size = 1; } else { - src_fast.reset(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd tuple")); + src_fast = pydynd::capture_if_not_null(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd tuple")); child_src = reinterpret_cast(PySequence_Fast_ITEMS(src_fast.get())); src_dim_size = PySequence_Fast_GET_SIZE(src_fast.get()); } @@ -652,7 +652,7 @@ struct assign_from_pyobject_kernel } else { // Get the input as an array of PyObject * - pydynd::pyobject_ownref src_fast; + pydynd::py_ref src_fast; char *child_src; intptr_t child_stride = sizeof(PyObject *); intptr_t src_dim_size; @@ -661,7 +661,7 @@ struct assign_from_pyobject_kernel src_dim_size = 1; } else { - src_fast.reset(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd struct")); + src_fast = pydynd::capture_if_not_null(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd struct")); child_src = reinterpret_cast(PySequence_Fast_ITEMS(src_fast.get())); src_dim_size = PySequence_Fast_GET_SIZE(src_fast.get()); } @@ -722,7 +722,7 @@ struct assign_from_pyobject_kernel dynd::kernel_strided_t copy_el_fn = copy_el->get_function(); // Get the input as an array of PyObject * - pydynd::pyobject_ownref src_fast; + pydynd::py_ref src_fast; char *child_src; intptr_t child_stride = sizeof(PyObject *); intptr_t src_dim_size; @@ -732,7 +732,8 @@ struct assign_from_pyobject_kernel src_dim_size = 1; } else { - src_fast.reset(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd dimension")); + src_fast = + pydynd::capture_if_not_null(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd dimension")); child_src = reinterpret_cast(PySequence_Fast_ITEMS(src_fast.get())); src_dim_size = PySequence_Fast_GET_SIZE(src_fast.get()); } @@ -791,7 +792,7 @@ struct assign_from_pyobject_kernel dynd::kernel_strided_t copy_el_fn = copy_el->get_function(); // Get the input as an array of PyObject * - pydynd::pyobject_ownref src_fast; + pydynd::py_ref src_fast; char *child_src; intptr_t child_stride = sizeof(PyObject *); intptr_t src_dim_size; @@ -800,7 +801,8 @@ struct assign_from_pyobject_kernel src_dim_size = 1; } else { - src_fast.reset(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd dimension")); + src_fast = + pydynd::capture_if_not_null(PySequence_Fast(src_obj, "Require a sequence to copy to a dynd dimension")); child_src = reinterpret_cast(PySequence_Fast_ITEMS(src_fast.get())); src_dim_size = PySequence_Fast_GET_SIZE(src_fast.get()); } diff --git a/dynd/include/kernels/assign_to_pyobject_kernel.hpp b/dynd/include/kernels/assign_to_pyobject_kernel.hpp index f5220592..4af4c429 100644 --- a/dynd/include/kernels/assign_to_pyobject_kernel.hpp +++ b/dynd/include/kernels/assign_to_pyobject_kernel.hpp @@ -92,10 +92,10 @@ PyObject *pyint_from_int(const dynd::uint128 &val) return PyLong_FromUnsignedLongLong(val.m_lo); } // Use the pynumber methods to shift and or together the 64 bit parts - pydynd::pyobject_ownref hi(PyLong_FromUnsignedLongLong(val.m_hi)); - pydynd::pyobject_ownref sixtyfour(PyLong_FromLong(64)); - pydynd::pyobject_ownref hi_shifted(PyNumber_Lshift(hi.get(), sixtyfour)); - pydynd::pyobject_ownref lo(PyLong_FromUnsignedLongLong(val.m_lo)); + pydynd::py_ref hi = pydynd::capture_if_not_null(PyLong_FromUnsignedLongLong(val.m_hi)); + pydynd::py_ref sixtyfour = pydynd::capture_if_not_null(PyLong_FromLong(64)); + pydynd::py_ref hi_shifted = pydynd::capture_if_not_null(PyNumber_Lshift(hi.get(), sixtyfour.get())); + pydynd::py_ref lo = pydynd::capture_if_not_null(PyLong_FromUnsignedLongLong(val.m_lo)); return PyNumber_Or(hi_shifted.get(), lo.get()); } @@ -105,7 +105,7 @@ PyObject *pyint_from_int(const dynd::int128 &val) if (val.m_hi == 0xffffffffffffffffULL && (val.m_hi & 0x8000000000000000ULL) != 0) { return PyLong_FromLongLong(static_cast(val.m_lo)); } - pydynd::pyobject_ownref absval(pyint_from_int(static_cast(-val))); + pydynd::py_ref absval = pydynd::capture_if_not_null(pyint_from_int(static_cast(-val))); return PyNumber_Negative(absval.get()); } else { @@ -420,7 +420,7 @@ struct assign_to_pyobject_kernel *dst_obj = NULL; intptr_t field_count = src_tp.extended()->get_field_count(); const uintptr_t *field_offsets = reinterpret_cast(src_arrmeta); - pydynd::pyobject_ownref tup(PyTuple_New(field_count)); + pydynd::py_ref tup = pydynd::capture_if_not_null(PyTuple_New(field_count)); for (intptr_t i = 0; i < field_count; ++i) { nd::kernel_prefix *copy_el = get_child(m_copy_el_offsets[i]); dynd::kernel_single_t copy_el_fn = copy_el->get_function(); @@ -431,7 +431,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = tup.release(); + *dst_obj = tup.get(); } }; @@ -442,7 +442,7 @@ struct assign_to_pyobject_kernel dynd::ndt::type m_src_tp; const char *m_src_arrmeta; std::vector m_copy_el_offsets; - pydynd::pyobject_ownref m_field_names; + pydynd::py_ref m_field_names; ~assign_to_pyobject_kernel() { @@ -458,19 +458,19 @@ struct assign_to_pyobject_kernel *dst_obj = NULL; intptr_t field_count = m_src_tp.extended()->get_field_count(); const uintptr_t *field_offsets = reinterpret_cast(m_src_arrmeta); - pydynd::pyobject_ownref dct(PyDict_New()); + pydynd::py_ref dct = pydynd::capture_if_not_null(PyDict_New()); for (intptr_t i = 0; i < field_count; ++i) { dynd::nd::kernel_prefix *copy_el = get_child(m_copy_el_offsets[i]); dynd::kernel_single_t copy_el_fn = copy_el->get_function(); char *el_src = src[0] + field_offsets[i]; - pydynd::pyobject_ownref el; - copy_el_fn(copy_el, reinterpret_cast(el.obj_addr()), &el_src); + pydynd::py_ref el; + copy_el_fn(copy_el, reinterpret_cast(&el), &el_src); PyDict_SetItem(dct.get(), PyTuple_GET_ITEM(m_field_names.get(), i), el.get()); } if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = dct.release(); + *dst_obj = dct.get(); } }; @@ -488,7 +488,7 @@ struct assign_to_pyobject_kernel PyObject **dst_obj = reinterpret_cast(dst); Py_XDECREF(*dst_obj); *dst_obj = NULL; - pydynd::pyobject_ownref lst(PyList_New(dim_size)); + pydynd::py_ref lst = pydynd::capture_if_not_null(PyList_New(dim_size)); nd::kernel_prefix *copy_el = get_child(); dynd::kernel_strided_t copy_el_fn = copy_el->get_function(); copy_el_fn(copy_el, reinterpret_cast(((PyListObject *)lst.get())->ob_item), sizeof(PyObject *), src, @@ -496,7 +496,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.release(); + *dst_obj = lst.get(); } }; @@ -515,7 +515,7 @@ struct assign_to_pyobject_kernel Py_XDECREF(*dst_obj); *dst_obj = NULL; const dynd::ndt::var_dim_type::data_type *vd = reinterpret_cast(src[0]); - pydynd::pyobject_ownref lst(PyList_New(vd->size)); + pydynd::py_ref lst = pydynd::capture_if_not_null(PyList_New(vd->size)); dynd::nd::kernel_prefix *copy_el = get_child(); dynd::kernel_strided_t copy_el_fn = copy_el->get_function(); char *el_src = vd->begin + offset; @@ -524,6 +524,6 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.release(); + *dst_obj = lst.get(); } }; diff --git a/dynd/include/numpy_type_interop.hpp b/dynd/include/numpy_type_interop.hpp index 356a7d14..701dc8c0 100644 --- a/dynd/include/numpy_type_interop.hpp +++ b/dynd/include/numpy_type_interop.hpp @@ -233,7 +233,7 @@ inline dynd::ndt::type _type_from_numpy_dtype(PyArray_Descr *d, size_t data_alig // Check for an h5py vlen string type (h5py 2.2 style) PyObject *vlen_tup = PyMapping_GetItemString(d->fields, "vlen"); if (vlen_tup != NULL) { - pyobject_ownref vlen_tup_owner(vlen_tup); + py_ref vlen_tup_owner = capture_if_not_null(vlen_tup); if (PyTuple_Check(vlen_tup) && PyTuple_GET_SIZE(vlen_tup) == 3) { PyObject *type_dict = PyTuple_GET_ITEM(vlen_tup, 2); if (PyDict_Check(type_dict)) { @@ -269,8 +269,9 @@ inline dynd::ndt::type _type_from_numpy_dtype(PyArray_Descr *d, size_t data_alig case NPY_DATETIME: { // Get the dtype info through the CPython API, slower // but lets NumPy's datetime API change without issue. - pyobject_ownref mod(PyImport_ImportModule("numpy")); - pyobject_ownref dd(PyObject_CallMethod(mod.get(), const_cast("datetime_data"), const_cast("O"), d)); + py_ref mod = capture_if_not_null(PyImport_ImportModule("numpy")); + py_ref dd = capture_if_not_null( + PyObject_CallMethod(mod.get(), const_cast("datetime_data"), const_cast("O"), d)); PyObject *unit = PyTuple_GetItem(dd.get(), 0); if (unit == NULL) { throw std::runtime_error(""); diff --git a/dynd/include/type_functions.hpp b/dynd/include/type_functions.hpp index 4005f2b6..a84c44cd 100644 --- a/dynd/include/type_functions.hpp +++ b/dynd/include/type_functions.hpp @@ -141,7 +141,7 @@ inline void pyobject_as_vector__type(PyObject *list_of_types, std::vector(PyObject *obj) noexcept } template -class py_ref { +class py_ref_tmpl { PyObject *o; public: @@ -111,7 +111,7 @@ class py_ref { // are defined after the class declaration. // If the class is allowed to be null, it default-initializes to null. // If the class is not allowed to be null, it default-initializes to None. - py_ref() noexcept; + py_ref_tmpl() noexcept; // First define an accessor to get the PyObject pointer from // the wrapper class. @@ -128,11 +128,11 @@ class py_ref { * this type doesn't allow null * and the input type doesn't allow null, * Then: - * Allow implicit conversions from the other py_ref type to this type. + * Allow implicit conversions from the other py_ref_tmpl type to this type. */ template * = nullptr> - py_ref(const py_ref &other) noexcept + py_ref_tmpl(const py_ref_tmpl &other) noexcept { o = other.o; incref_if_owned(o); @@ -142,10 +142,10 @@ class py_ref { * this type doesn't allow null, * and the input type does, * Then: - * Require that conversions from the other py_ref type to this type be explcit. + * Require that conversions from the other py_ref_tmpl type to this type be explcit. */ template * = nullptr> - explicit py_ref(const py_ref &other) noexcept + explicit py_ref_tmpl(const py_ref_tmpl &other) noexcept { o = other.o; incref_if_owned(o); @@ -162,7 +162,7 @@ class py_ref { * a move operation can be implicitly performed. */ template * = nullptr> - py_ref(py_ref &&other) noexcept + py_ref_tmpl(py_ref_tmpl &&other) noexcept { o = other.o; } @@ -174,7 +174,7 @@ class py_ref { * only an explicit move operation can be performed. */ template * = nullptr> - explicit py_ref(py_ref &&other) noexcept + explicit py_ref_tmpl(py_ref_tmpl &&other) noexcept { o = other.o; } @@ -187,7 +187,7 @@ class py_ref { * specify `consume_ref` as false regardless of whether or not you * own the reference represented in the PyObject* you pass in. */ - explicit py_ref(PyObject *obj, bool consume_ref) noexcept + explicit py_ref_tmpl(PyObject *obj, bool consume_ref) noexcept { o = obj; if (consume_ref) { @@ -198,7 +198,7 @@ class py_ref { } } - ~py_ref() { decref_if_owned(o); } + ~py_ref_tmpl() { decref_if_owned(o); } // For assignment operators, only allow assignment // in cases where implicit conversions are also allowed. @@ -213,11 +213,11 @@ class py_ref { * this type doesn't allow null * and the input type doesn't allow null, * Then: - * Allow assignment from the other py_ref type to this type. + * Allow assignment from the other py_ref_tmpl type to this type. */ template * = nullptr> - py_ref operator=(const py_ref &other) noexcept + py_ref_tmpl operator=(const py_ref_tmpl &other) noexcept { decref_if_owned(o); o = other.o; @@ -242,44 +242,42 @@ class py_ref { // Default constructors for various cases. template <> -inline py_ref::py_ref() noexcept +inline py_ref_tmpl::py_ref_tmpl() noexcept { o = Py_None; incref(o); } template <> -inline py_ref::py_ref() noexcept +inline py_ref_tmpl::py_ref_tmpl() noexcept { o = nullptr; } template <> -inline py_ref::py_ref() noexcept +inline py_ref_tmpl::py_ref_tmpl() noexcept { o = Py_None; } template <> -inline py_ref::py_ref() noexcept +inline py_ref_tmpl::py_ref_tmpl() noexcept { o = nullptr; } // Convenience aliases for the templated smart pointer classes. -// All the template arguments to py_ref have defaults, -// so py_ref can already be used for pointers that -// own their reference and cannot be null. -// The following aliases fill in the other cases. -using py_ref_with_null = py_ref; +using py_ref = py_ref_tmpl; -using py_borref = py_ref; +using py_ref_with_null = py_ref_tmpl; -using py_borref_with_null = py_ref; +using py_borref = py_ref_tmpl; + +using py_borref_with_null = py_ref_tmpl; // To help with the transition to the new classes. -using pyobject_ownref = py_ref; +using pyobject_ownref = py_ref_tmpl; /* Check if a wrapped pointer is null. * If it is not, return the pointer @@ -288,7 +286,7 @@ using pyobject_ownref = py_ref; * This can be used to forward exceptions from Python. */ template -py_ref check_null(py_ref &o) +py_ref_tmpl check_null(py_ref_tmpl &o) { if (o.is_null()) { throw std::runtime_error("Unexpected null pointer."); @@ -296,6 +294,17 @@ py_ref check_null(py_ref &o) return reinterpret_cast>(o); } +/* Capture a new reference if it is not null. + * Throw an exception if it is. + */ +inline py_ref capture_if_not_null(PyObject *o) +{ + if (o == nullptr) { + throw std::runtime_error("Unexpected null pouter."); + } + return py_ref(o, true); +} + class PyGILState_RAII { PyGILState_STATE m_gstate; @@ -332,7 +341,7 @@ inline void py_decref_function(void *obj) inline intptr_t pyobject_as_index(PyObject *index) { - pyobject_ownref start_obj(PyNumber_Index(index)); + py_ref start_obj = capture_if_not_null(PyNumber_Index(index)); intptr_t result; if (PyLong_Check(start_obj.get())) { result = PyLong_AsSsize_t(start_obj.get()); @@ -353,11 +362,11 @@ inline intptr_t pyobject_as_index(PyObject *index) inline int pyobject_as_int_index(PyObject *index) { - pyobject_ownref start_obj(PyNumber_Index(index)); + py_ref start_obj = capture_if_not_null(PyNumber_Index(index)); #if PY_VERSION_HEX >= 0x03000000 - long result = PyLong_AsLong(start_obj); + long result = PyLong_AsLong(start_obj.get()); #else - long result = PyInt_AsLong(start_obj); + long result = PyInt_AsLong(start_obj.get()); #endif if (result == -1 && PyErr_Occurred()) { throw std::exception(); @@ -394,7 +403,7 @@ inline std::string pystring_as_string(PyObject *str) char *data = NULL; Py_ssize_t len = 0; if (PyUnicode_Check(str)) { - pyobject_ownref utf8(PyUnicode_AsUTF8String(str)); + py_ref utf8 = capture_if_not_null(PyUnicode_AsUTF8String(str)); #if PY_VERSION_HEX >= 0x03000000 if (PyBytes_AsStringAndSize(utf8.get(), &data, &len) < 0) { @@ -436,7 +445,7 @@ inline PyObject *pystring_from_string(const std::string &str) } inline std::string pyobject_repr(PyObject *obj) { - pyobject_ownref src_repr(PyObject_Repr(obj)); + py_ref src_repr = capture_if_not_null(PyObject_Repr(obj)); return pystring_as_string(src_repr.get()); } @@ -445,7 +454,7 @@ inline void pyobject_as_vector_string(PyObject *list_string, std::vector Py_ssize_t size = PySequence_Size(list_index); vector_intp.resize(size); for (Py_ssize_t i = 0; i < size; ++i) { - pyobject_ownref item(PySequence_GetItem(list_index, i)); + py_ref item = capture_if_not_null(PySequence_GetItem(list_index, i)); vector_intp[i] = pyobject_as_index(item.get()); } } @@ -548,8 +557,8 @@ inline PyObject *intptr_array_as_tuple(size_t size, const intptr_t *values) // Helper function for pyarg_axis_argument. inline void mark_axis(PyObject *int_axis, int ndim, dynd::bool1 *reduce_axes) { - pyobject_ownref value_obj(PyNumber_Index(int_axis)); - long value = PyLong_AsLong(value_obj); + py_ref value_obj = capture_if_not_null(PyNumber_Index(int_axis)); + long value = PyLong_AsLong(value_obj.get()); if (value == -1 && PyErr_Occurred()) { throw std::runtime_error("error getting integer for axis argument"); } diff --git a/dynd/src/array_as_numpy.cpp b/dynd/src/array_as_numpy.cpp index f6c4d97c..0185d3e6 100644 --- a/dynd/src/array_as_numpy.cpp +++ b/dynd/src/array_as_numpy.cpp @@ -72,12 +72,11 @@ static int dynd_to_numpy_id(dynd::type_id_t id) } } -static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t ndim, const ndt::type &dt, - const char *arrmeta) +static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, const ndt::type &dt, const char *arrmeta) { // DyND builtin types if (dt.is_builtin()) { - out_numpy_dtype->reset((PyObject *)PyArray_DescrFromType(dynd_to_numpy_id(dt.get_id()))); + *out_numpy_dtype = capture_if_not_null((PyObject *)PyArray_DescrFromType(dynd_to_numpy_id(dt.get_id()))); return; } @@ -89,19 +88,19 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t case string_encoding_ascii: result = PyArray_DescrNewFromType(NPY_STRING); result->elsize = (int)fsd->get_data_size(); - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; case string_encoding_utf_32: result = PyArray_DescrNewFromType(NPY_UNICODE); result->elsize = (int)fsd->get_data_size(); - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; default: // If it's not one of the encodings NumPy supports, // use Unicode result = PyArray_DescrNewFromType(NPY_UNICODE); result->elsize = (int)fsd->get_data_size() * 4 / string_encoding_char_size_table[fsd->get_encoding()]; - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; } break; @@ -111,7 +110,7 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t PyArray_Descr *dtype = PyArray_DescrNewFromType(NPY_OBJECT); // Add metadata to the string type being created so that // it can round-trip. This metadata is compatible with h5py. - out_numpy_dtype->reset((PyObject *)dtype); + *out_numpy_dtype = capture_if_not_null((PyObject *)dtype); if (dtype->metadata == NULL) { dtype->metadata = PyDict_New(); } @@ -129,7 +128,7 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t // If this isn't one of the array dimensions, it maps into // a numpy dtype with a shape // Build up the shape of the array for NumPy - pyobject_ownref shape(PyList_New(0)); + py_ref shape = capture_if_not_null(PyList_New(0)); ndt::type element_tp = dt; while (ndim > 0) { const fixed_dim_type_arrmeta *am = reinterpret_cast(arrmeta); @@ -142,19 +141,19 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t } } // Get the numpy dtype of the element - pyobject_ownref child_numpy_dtype; + py_ref child_numpy_dtype; make_numpy_dtype_for_copy(&child_numpy_dtype, 0, element_tp, arrmeta); // Create the result numpy dtype - pyobject_ownref tuple_obj(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); + py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); + PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.get()); PyArray_Descr *result = NULL; - if (!PyArray_DescrConverter(tuple_obj, &result)) { + if (!PyArray_DescrConverter(tuple_obj.get(), &result)) { throw dynd::type_error("failed to convert dynd type into numpy subarray dtype"); } // Put the final numpy dtype reference in the output - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; } break; @@ -163,49 +162,49 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t const ndt::struct_type *bs = dt.extended(); size_t field_count = bs->get_field_count(); - pyobject_ownref names_obj(PyList_New(field_count)); + py_ref names_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { const dynd::string &fn = bs->get_field_name(i); #if PY_VERSION_HEX >= 0x03000000 - pyobject_ownref name_str(PyUnicode_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); + py_ref name_str = capture_if_not_null(PyUnicode_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #else - pyobject_ownref name_str(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); + py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, name_str.get()); } - pyobject_ownref formats_obj(PyList_New(field_count)); - pyobject_ownref offsets_obj(PyList_New(field_count)); + py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); + py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); size_t standard_offset = 0, standard_alignment = 1; for (size_t i = 0; i < field_count; ++i) { // Get the numpy dtype of the element - pyobject_ownref field_numpy_dtype; + py_ref field_numpy_dtype; make_numpy_dtype_for_copy(&field_numpy_dtype, 0, bs->get_field_type(i), arrmeta); size_t field_alignment = ((PyArray_Descr *)field_numpy_dtype.get())->alignment; size_t field_size = ((PyArray_Descr *)field_numpy_dtype.get())->elsize; standard_offset = inc_to_alignment(standard_offset, field_alignment); standard_alignment = max(standard_alignment, field_alignment); - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); - PyList_SET_ITEM((PyObject *)offsets_obj, i, PyLong_FromSize_t(standard_offset)); + PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.get()); + PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(standard_offset)); standard_offset += field_size; } // Get the full element size standard_offset = inc_to_alignment(standard_offset, standard_alignment); - pyobject_ownref itemsize_obj(PyLong_FromSize_t(standard_offset)); + py_ref itemsize_obj = capture_if_not_null(PyLong_FromSize_t(standard_offset)); - pyobject_ownref dict_obj(PyDict_New()); - PyDict_SetItemString(dict_obj, "names", names_obj); - PyDict_SetItemString(dict_obj, "formats", formats_obj); - PyDict_SetItemString(dict_obj, "offsets", offsets_obj); - PyDict_SetItemString(dict_obj, "itemsize", itemsize_obj); + py_ref dict_obj = capture_if_not_null(PyDict_New()); + PyDict_SetItemString(dict_obj.get(), "names", names_obj.get()); + PyDict_SetItemString(dict_obj.get(), "formats", formats_obj.get()); + PyDict_SetItemString(dict_obj.get(), "offsets", offsets_obj.get()); + PyDict_SetItemString(dict_obj.get(), "itemsize", itemsize_obj.get()); PyArray_Descr *result = NULL; - if (!PyArray_DescrAlignConverter(dict_obj, &result)) { + if (!PyArray_DescrAlignConverter(dict_obj.get(), &result)) { stringstream ss; ss << "failed to convert dynd type " << dt << " into numpy dtype via dict"; throw dynd::type_error(ss.str()); } - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; } default: { @@ -227,12 +226,12 @@ static void make_numpy_dtype_for_copy(pyobject_ownref *out_numpy_dtype, intptr_t throw dynd::type_error(ss.str()); } -static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requires_copy, intptr_t ndim, - const ndt::type &dt, const char *arrmeta) +static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, intptr_t ndim, const ndt::type &dt, + const char *arrmeta) { if (dt.is_builtin()) { // DyND builtin types - out_numpy_dtype->reset((PyObject *)PyArray_DescrFromType(dynd_to_numpy_id(dt.get_id()))); + *out_numpy_dtype = capture_if_not_null((PyObject *)PyArray_DescrFromType(dynd_to_numpy_id(dt.get_id()))); return; } switch (dt.get_id()) { @@ -243,15 +242,15 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir case string_encoding_ascii: result = PyArray_DescrNewFromType(NPY_STRING); result->elsize = (int)fsd->get_data_size(); - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; case string_encoding_utf_32: result = PyArray_DescrNewFromType(NPY_UNICODE); result->elsize = (int)fsd->get_data_size(); - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; default: - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -259,7 +258,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir } case string_id: { // Convert to numpy object type, requires copy - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -275,7 +274,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir else { // If this isn't one of the array dimensions, it maps into // a numpy dtype with a shape - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -295,7 +294,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir // If this isn't one of the array dimensions, it maps into // a numpy dtype with a shape // Build up the shape of the array for NumPy - pyobject_ownref shape(PyList_New(0)); + py_ref shape = capture_if_not_null(PyList_New(0)); ndt::type element_tp = dt; while (ndim > 0) { size_t dim_size = 0; @@ -306,7 +305,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir if (cfd->get_data_size() != element_tp.get_data_size() * dim_size) { // If it's not C-order, a copy is required - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -323,19 +322,19 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir } } // Get the numpy dtype of the element - pyobject_ownref child_numpy_dtype; + py_ref child_numpy_dtype; as_numpy_analysis(&child_numpy_dtype, out_requires_copy, 0, element_tp, arrmeta); if (*out_requires_copy) { // If the child required a copy, stop right away - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); return; } // Create the result numpy dtype - pyobject_ownref tuple_obj(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); + py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); + PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.get()); PyArray_Descr *result = NULL; if (!PyArray_DescrConverter(tuple_obj, &result)) { @@ -343,7 +342,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir "failed to convert dynd type into numpy subarray dtype"); } // Put the final numpy dtype reference in the output - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; } break; @@ -352,7 +351,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir case struct_id: { if (dt.get_id() == struct_id && arrmeta == NULL) { // If it's a struct type with no arrmeta, a copy is required - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -360,51 +359,51 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir const uintptr_t *offsets = reinterpret_cast(arrmeta); size_t field_count = bs->get_field_count(); - pyobject_ownref names_obj(PyList_New(field_count)); + py_ref names_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { const dynd::string &fn = bs->get_field_name(i); #if PY_VERSION_HEX >= 0x03000000 - pyobject_ownref name_str(PyUnicode_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); + py_ref name_str = capture_if_not_null(PyUnicode_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #else - pyobject_ownref name_str(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); + py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, name_str.get()); } - pyobject_ownref formats_obj(PyList_New(field_count)); + py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { // Get the numpy dtype of the element - pyobject_ownref field_numpy_dtype; + py_ref field_numpy_dtype; as_numpy_analysis(&field_numpy_dtype, out_requires_copy, 0, bs->get_field_type(i), arrmeta); if (*out_requires_copy) { // If the field required a copy, stop right away - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); return; } - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); + PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.get()); } - pyobject_ownref offsets_obj(PyList_New(field_count)); + py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { - PyList_SET_ITEM((PyObject *)offsets_obj, i, PyLong_FromSize_t(offsets[i])); + PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(offsets[i])); } - pyobject_ownref dict_obj(PyDict_New()); - PyDict_SetItemString(dict_obj, "names", names_obj); - PyDict_SetItemString(dict_obj, "formats", formats_obj); - PyDict_SetItemString(dict_obj, "offsets", offsets_obj); + py_ref dict_obj = capture_if_not_null(PyDict_New()); + PyDict_SetItemString(dict_obj.get(), "names", names_obj.get()); + PyDict_SetItemString(dict_obj.get(), "formats", formats_obj.get()); + PyDict_SetItemString(dict_obj.get(), "offsets", offsets_obj.get()); if (dt.get_data_size() > 0) { - pyobject_ownref itemsize_obj(PyLong_FromSize_t(dt.get_data_size())); - PyDict_SetItemString(dict_obj, "itemsize", itemsize_obj); + py_ref itemsize_obj = capture_if_not_null(PyLong_FromSize_t(dt.get_data_size())); + PyDict_SetItemString(dict_obj.get(), "itemsize", itemsize_obj.get()); } PyArray_Descr *result = NULL; - if (!PyArray_DescrConverter(dict_obj, &result)) { + if (!PyArray_DescrConverter(dict_obj.get(), &result)) { stringstream ss; ss << "failed to convert dynd type " << dt << " into numpy dtype via dict"; throw dynd::type_error(ss.str()); } - out_numpy_dtype->reset((PyObject *)result); + *out_numpy_dtype = capture_if_not_null((PyObject *)result); return; } default: { @@ -415,7 +414,7 @@ static void as_numpy_analysis(pyobject_ownref *out_numpy_dtype, bool *out_requir if (dt.get_base_id() == expr_kind_id) { // If none of the prior checks caught this expression, // a copy is required. - out_numpy_dtype->clear(); + *out_numpy_dtype = py_ref(Py_None, false); *out_requires_copy = true; return; } @@ -440,7 +439,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) // If a copy is allowed, convert the builtin scalars to NumPy scalars if (allow_copy && a.get_type().is_scalar()) { - pyobject_ownref result; + py_ref result; switch (a.get_type().get_id()) { case uninitialized_id: throw runtime_error("cannot convert uninitialized dynd array to numpy"); @@ -449,60 +448,60 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) case bool_id: if (*a.cdata()) { Py_INCREF(PyArrayScalar_True); - result.reset(PyArrayScalar_True); + result = capture_if_not_null(PyArrayScalar_True); } else { Py_INCREF(PyArrayScalar_False); - result.reset(PyArrayScalar_False); + result = capture_if_not_null(PyArrayScalar_False); } break; case int8_id: - result.reset(PyArrayScalar_New(Int8)); + result = capture_if_not_null(PyArrayScalar_New(Int8)); PyArrayScalar_ASSIGN(result.get(), Int8, *reinterpret_cast(a.cdata())); break; case int16_id: - result.reset(PyArrayScalar_New(Int16)); + result = capture_if_not_null(PyArrayScalar_New(Int16)); PyArrayScalar_ASSIGN(result.get(), Int16, *reinterpret_cast(a.cdata())); break; case int32_id: - result.reset(PyArrayScalar_New(Int32)); + result = capture_if_not_null(PyArrayScalar_New(Int32)); PyArrayScalar_ASSIGN(result.get(), Int32, *reinterpret_cast(a.cdata())); break; case int64_id: - result.reset(PyArrayScalar_New(Int64)); + result = capture_if_not_null(PyArrayScalar_New(Int64)); PyArrayScalar_ASSIGN(result.get(), Int64, *reinterpret_cast(a.cdata())); break; case uint8_id: - result.reset(PyArrayScalar_New(UInt8)); + result = capture_if_not_null(PyArrayScalar_New(UInt8)); PyArrayScalar_ASSIGN(result.get(), UInt8, *reinterpret_cast(a.cdata())); break; case uint16_id: - result.reset(PyArrayScalar_New(UInt16)); + result = capture_if_not_null(PyArrayScalar_New(UInt16)); PyArrayScalar_ASSIGN(result.get(), UInt16, *reinterpret_cast(a.cdata())); break; case uint32_id: - result.reset(PyArrayScalar_New(UInt32)); + result = capture_if_not_null(PyArrayScalar_New(UInt32)); PyArrayScalar_ASSIGN(result.get(), UInt32, *reinterpret_cast(a.cdata())); break; case uint64_id: - result.reset(PyArrayScalar_New(UInt64)); + result = capture_if_not_null(PyArrayScalar_New(UInt64)); PyArrayScalar_ASSIGN(result.get(), UInt64, *reinterpret_cast(a.cdata())); break; case float32_id: - result.reset(PyArrayScalar_New(Float32)); + result = capture_if_not_null(PyArrayScalar_New(Float32)); PyArrayScalar_ASSIGN(result.get(), Float32, *reinterpret_cast(a.cdata())); break; case float64_id: - result.reset(PyArrayScalar_New(Float64)); + result = capture_if_not_null(PyArrayScalar_New(Float64)); PyArrayScalar_ASSIGN(result.get(), Float64, *reinterpret_cast(a.cdata())); break; case complex_float32_id: - result.reset(PyArrayScalar_New(Complex64)); + result = capture_if_not_null(PyArrayScalar_New(Complex64)); PyArrayScalar_VAL(result.get(), Complex64).real = reinterpret_cast(a.cdata())[0]; PyArrayScalar_VAL(result.get(), Complex64).imag = reinterpret_cast(a.cdata())[1]; break; case complex_float64_id: - result.reset(PyArrayScalar_New(Complex128)); + result = capture_if_not_null(PyArrayScalar_New(Complex128)); PyArrayScalar_VAL(result.get(), Complex128).real = reinterpret_cast(a.cdata())[0]; PyArrayScalar_VAL(result.get(), Complex128).imag = reinterpret_cast(a.cdata())[1]; break; @@ -512,7 +511,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) // make copies of strings if (a.get_type().get_base_id() == expr_kind_id) { // If it's an expression kind - pyobject_ownref n_tmp(pydynd::array_from_cpp(a.eval())); + py_ref n_tmp = capture_if_not_null(pydynd::array_from_cpp(a.eval())); return array_as_numpy(n_tmp.get(), true); } else if (a.get_type().get_base_id() == string_kind_id) { @@ -529,12 +528,12 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) throw dynd::type_error(ss.str()); } } - return result.release(); + return result.get(); } if (a.get_type().get_id() == var_dim_id) { // If it's a var_dim, view it as fixed then try again - pyobject_ownref n_tmp(pydynd::array_from_cpp(a.view( + py_ref n_tmp = capture_if_not_null(pydynd::array_from_cpp(a.view( ndt::make_fixed_dim(a.get_dim_size(), a.get_type().extended()->get_element_type())))); return array_as_numpy(n_tmp.get(), allow_copy); } @@ -544,7 +543,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) // Do a recursive analysis of the dynd array for how to // convert it to NumPy bool requires_copy = false; - pyobject_ownref numpy_dtype; + py_ref numpy_dtype; size_t ndim = a.get_ndim(); dimvector shape(ndim), strides(ndim); @@ -572,17 +571,17 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) } // Create a new NumPy array, and copy from the dynd array - pyobject_ownref result(PyArray_NewFromDescr(&PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), (int)ndim, - shape.get(), strides.get(), NULL, 0, NULL)); + py_ref result = capture_if_not_null(PyArray_NewFromDescr(&PyArray_Type, (PyArray_Descr *)numpy_dtype.get(), + (int)ndim, shape.get(), strides.get(), NULL, 0, NULL)); array_copy_to_numpy((PyArrayObject *)result.get(), a.get_type(), a.get()->metadata(), a.cdata()); // Return the NumPy array - return result.release(); + return result.get(); } else { // Create a view directly to the dynd array - pyobject_ownref result(PyArray_NewFromDescr( - &PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), (int)ndim, shape.get(), strides.get(), + py_ref result = capture_if_not_null(PyArray_NewFromDescr( + &PyArray_Type, (PyArray_Descr *)numpy_dtype.get(), (int)ndim, shape.get(), strides.get(), const_cast(a.cdata()), ((a.get_flags() & nd::write_access_flag) ? NPY_ARRAY_WRITEABLE : 0) | NPY_ARRAY_ALIGNED, NULL)); @@ -595,7 +594,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) PyArray_BASE(result.get()) = n_obj; Py_INCREF(n_obj); #endif - return result.release(); + return result.get(); } } diff --git a/dynd/src/array_from_py.cpp b/dynd/src/array_from_py.cpp index 17352074..40c66392 100644 --- a/dynd/src/array_from_py.cpp +++ b/dynd/src/array_from_py.cpp @@ -124,7 +124,7 @@ inline void convert_one_pyscalar_ustring(const ndt::type &tp, const char *arrmet dynd::string *out_usp = reinterpret_cast(out); if (PyUnicode_Check(obj)) { // Get it as UTF8 - pyobject_ownref utf8(PyUnicode_AsUTF8String(obj)); + py_ref utf8 = capture_if_not_null(PyUnicode_AsUTF8String(obj)); char *s = NULL; Py_ssize_t len = 0; if (PyBytes_AsStringAndSize(utf8.get(), &s, &len) < 0) { @@ -442,7 +442,7 @@ dynd::nd::array pydynd::array_from_py(PyObject *obj, uint32_t access_flags, bool #endif } else if (PyUnicode_Check(obj)) { - pyobject_ownref utf8(PyUnicode_AsUTF8String(obj)); + py_ref utf8 = capture_if_not_null(PyUnicode_AsUTF8String(obj)); char *s = NULL; Py_ssize_t len = 0; if (PyBytes_AsStringAndSize(utf8.get(), &s, &len) < 0) { @@ -467,7 +467,7 @@ dynd::nd::array pydynd::array_from_py(PyObject *obj, uint32_t access_flags, bool } if (result.get() == NULL) { - pyobject_ownref pytpstr(PyObject_Str((PyObject *)Py_TYPE(obj))); + py_ref pytpstr = capture_if_not_null(PyObject_Str((PyObject *)Py_TYPE(obj))); stringstream ss; ss << "could not convert python object of type "; ss << pystring_as_string(pytpstr.get()); diff --git a/dynd/src/numpy_interop.cpp b/dynd/src/numpy_interop.cpp index 6730fed3..ec9f96a8 100644 --- a/dynd/src/numpy_interop.cpp +++ b/dynd/src/numpy_interop.cpp @@ -199,12 +199,12 @@ dynd::nd::array pydynd::array_from_numpy_scalar(PyObject *obj, uint32_t access_f result = dynd::nd::array(dynd::complex(val.real, val.imag)); } else if (PyArray_IsScalar(obj, Void)) { - pyobject_ownref arr(PyArray_FromAny(obj, NULL, 0, 0, 0, NULL)); + py_ref arr = capture_if_not_null(PyArray_FromAny(obj, NULL, 0, 0, 0, NULL)); return array_from_numpy_array((PyArrayObject *)arr.get(), access_flags, true); } else { stringstream ss; - pyobject_ownref obj_tp(PyObject_Repr((PyObject *)Py_TYPE(obj))); + py_ref obj_tp = capture_if_not_null(PyObject_Repr((PyObject *)Py_TYPE(obj))); ss << "could not create a dynd array from the numpy scalar object"; ss << " of type " << pydynd::pystring_as_string(obj_tp.get()); throw dynd::type_error(ss.str()); diff --git a/dynd/src/numpy_type_interop.cpp b/dynd/src/numpy_type_interop.cpp index a43240d9..125875e6 100644 --- a/dynd/src/numpy_type_interop.cpp +++ b/dynd/src/numpy_type_interop.cpp @@ -69,29 +69,29 @@ PyArray_Descr *pydynd::numpy_dtype_from__type(const dynd::ndt::type &tp) size_t num_fields = fields.size(); const vector& offsets = ttp->get_offsets(); // TODO: Deal with the names better - pyobject_ownref names_obj(PyList_New(num_fields)); + py_ref names_obj = capture_if_not_null(PyList_New(num_fields)); for (size_t i = 0; i < num_fields; ++i) { stringstream ss; ss << "f" << i; - PyList_SET_ITEM((PyObject *)names_obj, i, + PyList_SET_ITEM(names_obj.get(), i, PyString_FromString(ss.str().c_str())); } - pyobject_ownref formats_obj(PyList_New(num_fields)); + py_ref formats_obj = capture_if_not_null(PyList_New(num_fields)); for (size_t i = 0; i < num_fields; ++i) { - PyList_SET_ITEM((PyObject *)formats_obj, i, (PyObject + PyList_SET_ITEM(formats_obj.get(), i, (PyObject *)numpy_dtype_from__type(fields[i])); } - pyobject_ownref offsets_obj(PyList_New(num_fields)); + py_ref offsets_obj = capture_if_not_null(PyList_New(num_fields)); for (size_t i = 0; i < num_fields; ++i) { - PyList_SET_ITEM((PyObject *)offsets_obj, i, + PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(offsets[i])); } - pyobject_ownref itemsize_obj(PyLong_FromSize_t(tp.get_data_size())); - pyobject_ownref dict_obj(PyDict_New()); - PyDict_SetItemString(dict_obj, "names", names_obj); - PyDict_SetItemString(dict_obj, "formats", formats_obj); - PyDict_SetItemString(dict_obj, "offsets", offsets_obj); - PyDict_SetItemString(dict_obj, "itemsize", itemsize_obj); + py_ref itemsize_obj = capture_if_not_null(PyLong_FromSize_t(tp.get_data_size())); + py_ref dict_obj = capture_if_not_null(PyDict_New()); + PyDict_SetItemString(dict_obj.get(), "names", names_obj.get()); + PyDict_SetItemString(dict_obj.get(), "formats", formats_obj.get()); + PyDict_SetItemString(dict_obj.get(), "offsets", offsets_obj.get()); + PyDict_SetItemString(dict_obj.get(), "itemsize", itemsize_obj.get()); PyArray_Descr *result = NULL; if (PyArray_DescrConverter(dict_obj, &result) != NPY_SUCCEED) { throw dynd::type_error("failed to convert tuple dtype into numpy @@ -106,19 +106,19 @@ dtype via dict"); std::vector offsets(field_count); struct_type::fill_default_data_offsets(field_count, ttp->get_field_types_raw(), offsets.data()); - pyobject_ownref names_obj(PyList_New(field_count)); + py_ref names_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { const string_type_data& fname = ttp->get_field_name(i); #if PY_VERSION_HEX >= 0x03000000 - pyobject_ownref name_str(PyUnicode_FromStringAndSize( + py_ref name_str = capture_if_not_null(PyUnicode_FromStringAndSize( fname.begin, fname.end - fname.begin)); #else - pyobject_ownref name_str(PyString_FromStringAndSize( + py_ref name_str = capture_if_not_null(PyString_FromStringAndSize( fname.begin, fname.end - fname.begin)); #endif - PyList_SET_ITEM((PyObject *)names_obj, i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, name_str.get()); } - pyobject_ownref formats_obj(PyList_New(field_count)); + py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { PyArray_Descr *npdt = numpy_dtype_from__type(ttp->get_field_type(i)); @@ -126,27 +126,27 @@ numpy_dtype_from__type(ttp->get_field_type(i)); (size_t)npdt->alignment); PyList_SET_ITEM((PyObject *)formats_obj, i, (PyObject *)npdt); } - pyobject_ownref offsets_obj(PyList_New(field_count)); + py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { PyList_SET_ITEM((PyObject *)offsets_obj, i, PyLong_FromSize_t(offsets[i])); } - pyobject_ownref -itemsize_obj(PyLong_FromSize_t(tp.get_default_data_size())); - pyobject_ownref dict_obj(PyDict_New()); - PyDict_SetItemString(dict_obj, "names", names_obj); - PyDict_SetItemString(dict_obj, "formats", formats_obj); - PyDict_SetItemString(dict_obj, "offsets", offsets_obj); - PyDict_SetItemString(dict_obj, "itemsize", itemsize_obj); + py_ref itemsize_obj = + capture_if_not_null(PyLong_FromSize_t(tp.get_default_data_size())); + py_ref dict_obj = capture_if_not_null(PyDict_New()); + PyDict_SetItemString(dict_obj.get(), "names", names_obj.get()); + PyDict_SetItemString(dict_obj.get(), "formats", formats_obj.get()); + PyDict_SetItemString(dict_obj.get(), "offsets", offsets_obj.get()); + PyDict_SetItemString(dict_obj.get(), "itemsize", itemsize_obj.get()); // This isn't quite right, but the rules between numpy and dynd // differ enough to make this tricky. if (max_numpy_alignment > 1 && max_numpy_alignment == tp.get_data_alignment()) { Py_INCREF(Py_True); - PyDict_SetItemString(dict_obj, "aligned", Py_True); + PyDict_SetItemString(dict_obj.get(), "aligned", Py_True); } PyArray_Descr *result = NULL; - if (PyArray_DescrConverter(dict_obj, &result) != NPY_SUCCEED) { + if (PyArray_DescrConverter(dict_obj.get(), &result) != NPY_SUCCEED) { stringstream ss; ss << "failed to convert dtype " << tp << " into numpy dtype via dict"; @@ -170,13 +170,13 @@ dtype because it is not C-order"; } child_tp = ttp->get_element_type(); } while (child_tp.get_id() == cfixed_dim_id); - pyobject_ownref dtype_obj((PyObject + py_ref dtype_obj = capture_if_not_null((PyObject *)numpy_dtype_from__type(child_tp)); - pyobject_ownref shape_obj(intptr_array_as_tuple((int)shape.size(), + py_ref shape_obj = capture_if_not_null(intptr_array_as_tuple((int)shape.size(), &shape[0])); - pyobject_ownref tuple_obj(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, dtype_obj.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape_obj.release()); + py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); + PyTuple_SET_ITEM(tuple_obj.get(), 0, dtype_obj.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape_obj.get()); PyArray_Descr *result = NULL; if (PyArray_DescrConverter(tuple_obj, &result) != NPY_SUCCEED) { throw dynd::type_error("failed to convert dynd type into numpy @@ -210,45 +210,45 @@ PyArray_Descr *pydynd::numpy_dtype_from__type(const dynd::ndt::type &tp, const c size_t field_count = stp->get_field_count(); size_t max_numpy_alignment = 1; - pyobject_ownref names_obj(PyList_New(field_count)); + py_ref names_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { const dynd::string &fname = stp->get_field_name(i); #if PY_VERSION_HEX >= 0x03000000 - pyobject_ownref name_str(PyUnicode_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); + py_ref name_str = capture_if_not_null(PyUnicode_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); #else - pyobject_ownref name_str(PyString_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); + py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); #endif - PyList_SET_ITEM((PyObject *)names_obj, i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, name_str.get()); } - pyobject_ownref formats_obj(PyList_New(field_count)); + py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { PyArray_Descr *npdt = numpy_dtype_from__type(stp->get_field_type(i), arrmeta + arrmeta_offsets[i]); max_numpy_alignment = max(max_numpy_alignment, (size_t)npdt->alignment); - PyList_SET_ITEM((PyObject *)formats_obj, i, (PyObject *)npdt); + PyList_SET_ITEM(formats_obj.get(), i, (PyObject *)npdt); } - pyobject_ownref offsets_obj(PyList_New(field_count)); + py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { - PyList_SET_ITEM((PyObject *)offsets_obj, i, PyLong_FromSize_t(offsets[i])); + PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(offsets[i])); } - pyobject_ownref itemsize_obj(PyLong_FromSize_t(tp.get_data_size())); + py_ref itemsize_obj = capture_if_not_null(PyLong_FromSize_t(tp.get_data_size())); - pyobject_ownref dict_obj(PyDict_New()); - PyDict_SetItemString(dict_obj, "names", names_obj); - PyDict_SetItemString(dict_obj, "formats", formats_obj); - PyDict_SetItemString(dict_obj, "offsets", offsets_obj); - PyDict_SetItemString(dict_obj, "itemsize", itemsize_obj); + py_ref dict_obj = capture_if_not_null(PyDict_New()); + PyDict_SetItemString(dict_obj.get(), "names", names_obj.get()); + PyDict_SetItemString(dict_obj.get(), "formats", formats_obj.get()); + PyDict_SetItemString(dict_obj.get(), "offsets", offsets_obj.get()); + PyDict_SetItemString(dict_obj.get(), "itemsize", itemsize_obj.get()); // This isn't quite right, but the rules between numpy and dynd // differ enough to make this tricky. if (max_numpy_alignment > 1 && max_numpy_alignment == tp.get_data_alignment()) { Py_INCREF(Py_True); - PyDict_SetItemString(dict_obj, "aligned", Py_True); + PyDict_SetItemString(dict_obj.get(), "aligned", Py_True); } PyArray_Descr *result = NULL; - if (PyArray_DescrConverter(dict_obj, &result) != NPY_SUCCEED) { + if (PyArray_DescrConverter(dict_obj.get(), &result) != NPY_SUCCEED) { throw dynd::type_error("failed to convert dtype into numpy struct dtype via dict"); } return result; @@ -445,7 +445,7 @@ dynd::ndt::type pydynd::array_from_numpy_scalar2(PyObject *obj) } stringstream ss; - pyobject_ownref obj_tp(PyObject_Repr((PyObject *)Py_TYPE(obj))); + py_ref obj_tp = capture_if_not_null(PyObject_Repr((PyObject *)Py_TYPE(obj))); ss << "could not create a dynd array from the numpy scalar object"; ss << " of type " << pydynd::pystring_as_string(obj_tp.get()); throw dynd::type_error(ss.str()); diff --git a/dynd/src/type_deduction.cpp b/dynd/src/type_deduction.cpp index 7c44a51e..6f8273da 100644 --- a/dynd/src/type_deduction.cpp +++ b/dynd/src/type_deduction.cpp @@ -89,7 +89,7 @@ void pydynd::deduce_pyseq_shape(PyObject *obj, size_t ndim, intptr_t *shape) if (ndim > 1) { for (Py_ssize_t i = 0; i < size; ++i) { - pyobject_ownref item(PySequence_GetItem(obj, i)); + py_ref item = capture_if_not_null(PySequence_GetItem(obj, i)); deduce_pyseq_shape(item.get(), ndim - 1, shape + 1); } } @@ -157,7 +157,7 @@ void pydynd::deduce_pyseq_shape_using_dtype(PyObject *obj, const ndt::type &tp, } for (Py_ssize_t i = 0; i < size; ++i) { - pyobject_ownref item(PySequence_GetItem(obj, i)); + py_ref item = capture_if_not_null(PySequence_GetItem(obj, i)); deduce_pyseq_shape_using_dtype(item.get(), tp, shape, i == 0 && initial_pass, current_axis + 1); } } @@ -213,11 +213,13 @@ bool pydynd::broadcast_as_scalar(const dynd::ndt::type &tp, PyObject *obj) intptr_t obj_ndim = 0; // Estimate the number of dimensions in ``obj`` by repeatedly indexing // along zero - pyobject_ownref v(obj); - Py_INCREF(v); + py_ref v = capture_if_not_null(obj); + // incref the object since we just claimed there was a reference there to capture, + // even though this function really just takes a borrowed reference. + Py_INCREF(v.get()); for (;;) { // Don't treat these types as sequences - if (PyDict_Check(v)) { + if (PyDict_Check(v.get())) { if (tp.get_dtype().get_id() == struct_id) { // If the object to assign to a dynd struct ends in a dict, apply // the dict as the struct's value @@ -225,10 +227,10 @@ bool pydynd::broadcast_as_scalar(const dynd::ndt::type &tp, PyObject *obj) } break; } - else if (PyUnicode_Check(v) || PyBytes_Check(v)) { + else if (PyUnicode_Check(v.get()) || PyBytes_Check(v.get())) { break; } - PyObject *iter = PyObject_GetIter(v); + PyObject *iter = PyObject_GetIter(v.get()); if (iter != NULL) { ++obj_ndim; if (iter == v.get()) { @@ -238,7 +240,7 @@ bool pydynd::broadcast_as_scalar(const dynd::ndt::type &tp, PyObject *obj) return false; } else { - pyobject_ownref iter_owner(iter); + py_ref iter_owner = capture_if_not_null(iter); PyObject *item = PyIter_Next(iter); if (item == NULL) { if (PyErr_ExceptionMatches(PyExc_StopIteration)) { @@ -250,7 +252,7 @@ bool pydynd::broadcast_as_scalar(const dynd::ndt::type &tp, PyObject *obj) } } else { - v.reset(item); + v = capture_if_not_null(item); } } } From 365835470389f8af771a8e8ba8b3e3fe9ea99022 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Fri, 3 Jun 2016 10:13:46 -0600 Subject: [PATCH 03/18] Fix MSVC compilation error. The order of the conditions in enable_if_t matters since, in some cases, the compiler uses short circuit evaluation of the compile-time constant expressions to simplify the enable_if condition so that it no longer depends on the inputs and only depends on the template parameters of the class being constructed, which, then, causes an error since that isn't allowed. --- dynd/include/utility_functions.hpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index ade81c63..e8b2cfba 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -144,7 +144,7 @@ class py_ref_tmpl { * Then: * Require that conversions from the other py_ref_tmpl type to this type be explcit. */ - template * = nullptr> + template * = nullptr> explicit py_ref_tmpl(const py_ref_tmpl &other) noexcept { o = other.o; @@ -161,7 +161,7 @@ class py_ref_tmpl { * Then: * a move operation can be implicitly performed. */ - template * = nullptr> + template * = nullptr> py_ref_tmpl(py_ref_tmpl &&other) noexcept { o = other.o; @@ -173,7 +173,7 @@ class py_ref_tmpl { * Then: * only an explicit move operation can be performed. */ - template * = nullptr> + template * = nullptr> explicit py_ref_tmpl(py_ref_tmpl &&other) noexcept { o = other.o; @@ -216,7 +216,7 @@ class py_ref_tmpl { * Allow assignment from the other py_ref_tmpl type to this type. */ template * = nullptr> + typename std::enable_if_t<(other_not_null && not_null) || !not_null> * = nullptr> py_ref_tmpl operator=(const py_ref_tmpl &other) noexcept { decref_if_owned(o); From acca45b85ae8cde75171532a9ce9882976ef8376 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Sat, 4 Jun 2016 14:50:05 -0600 Subject: [PATCH 04/18] Rewrite smart pointers that encapsulate Python references. First, Rewrite non-null pointer types to use asserts rather than an absolute assumption that the pointer type is not null. Second, add move assignment operators to avoid unneeded incref/decrefs when assigning between these smart pointer classes. Third, allow implicit conversions to happen between all four of these classes. Conversions from pointers that may be null to pointers that count null as an uninitialized state now raise an exception if a null value is encountered, and all other constructors are marked as noexcept. Fourth, add a method to release an owned reference from a given pointer. This replaces the old release method and eliminates various segfaults that were due to over-agressive calls to decref. --- .../callables/assign_to_pyobject_callable.hpp | 2 +- .../kernels/assign_to_pyobject_kernel.hpp | 8 +- dynd/include/utility_functions.hpp | 253 ++++++++++++------ dynd/nd/array.pyx | 2 +- dynd/src/array_as_numpy.cpp | 26 +- dynd/src/numpy_type_interop.cpp | 8 +- 6 files changed, 190 insertions(+), 109 deletions(-) diff --git a/dynd/include/callables/assign_to_pyobject_callable.hpp b/dynd/include/callables/assign_to_pyobject_callable.hpp index c0ebe26f..f259ba15 100644 --- a/dynd/include/callables/assign_to_pyobject_callable.hpp +++ b/dynd/include/callables/assign_to_pyobject_callable.hpp @@ -294,7 +294,7 @@ namespace nd { const dynd::string &rawname = src0_tp.extended()->get_field_name(i); pydynd::py_ref name = capture_if_not_null(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); - PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.get()); + PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.release()); } self_ck->m_copy_el_offsets.resize(field_count); diff --git a/dynd/include/kernels/assign_to_pyobject_kernel.hpp b/dynd/include/kernels/assign_to_pyobject_kernel.hpp index 4af4c429..4ca5e992 100644 --- a/dynd/include/kernels/assign_to_pyobject_kernel.hpp +++ b/dynd/include/kernels/assign_to_pyobject_kernel.hpp @@ -431,7 +431,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = tup.get(); + *dst_obj = tup.release(); } }; @@ -470,7 +470,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = dct.get(); + *dst_obj = dct.release(); } }; @@ -496,7 +496,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.get(); + *dst_obj = lst.release(); } }; @@ -524,6 +524,6 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.get(); + *dst_obj = lst.release(); } }; diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index e8b2cfba..ba77a7b3 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -15,6 +15,9 @@ #include "visibility.hpp" +// if if_condition is true, then assert assert_condition. +#define PYDYND_ASSERT_IF(if_condition, assert_condition) (assert((!(if_condition)) || (assert_condition))) + namespace dynd { // Forward declaration struct callable_type_data; @@ -34,6 +37,7 @@ inline void incref(PyObject *obj) noexcept template <> inline void incref(PyObject *obj) noexcept { + assert(obj != nullptr); Py_INCREF(obj); } @@ -49,6 +53,7 @@ inline void decref(PyObject *obj) noexcept template <> inline void decref(PyObject *obj) noexcept { + assert(obj != nullptr); Py_DECREF(obj); } @@ -58,6 +63,7 @@ inline void incref_if_owned(PyObject *obj) noexcept; template <> inline void incref_if_owned(PyObject *obj) noexcept { + assert(obj != nullptr); Py_INCREF(obj); } @@ -83,6 +89,7 @@ inline void decref_if_owned(PyObject *obj) noexcept; template <> inline void decref_if_owned(PyObject *obj) noexcept { + assert(obj != nullptr); Py_DECREF(obj); } @@ -107,20 +114,23 @@ class py_ref_tmpl { PyObject *o; public: - // Explicit specializations for default constructor - // are defined after the class declaration. - // If the class is allowed to be null, it default-initializes to null. - // If the class is not allowed to be null, it default-initializes to None. - py_ref_tmpl() noexcept; + // All versions default-initialize to null. + // This allows the smart pointer class to be moved from. + // Whether or not the class is allowed to be null only changes + // when zerochecks/exceptions may occur and when assertions are used. + py_ref_tmpl() noexcept : o(nullptr){}; // First define an accessor to get the PyObject pointer from // the wrapper class. - PyObject *get() noexcept { return o; } + PyObject *get() noexcept + { + PYDYND_ASSERT_IF(not_null, o != nullptr); + return o; + } - // All copy and move constructors are defined as noexcept. - // If the conversion or move operation would move from a type - // that can be null to one that can not, the corresponding - // constructor is declared explicit. + // All copy and move constructors are designated implicit. + // Unless they convert between a null type to a not_null type, + // they are also declared noexcept. /* If: * this type allows null, @@ -128,55 +138,88 @@ class py_ref_tmpl { * this type doesn't allow null * and the input type doesn't allow null, * Then: - * Allow implicit conversions from the other py_ref_tmpl type to this type. + * Conversions from the other py_ref_tmpl type to this type do not raise exceptions. */ - template * = nullptr> - py_ref_tmpl(const py_ref_tmpl &other) noexcept + template * = nullptr> + py_ref_tmpl(const py_ref_tmpl &other) noexcept { + PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); o = other.o; incref_if_owned(o); } + // Should this one be declared explicit since it can throw? /* If: * this type doesn't allow null, * and the input type does, * Then: - * Require that conversions from the other py_ref_tmpl type to this type be explcit. + * Allow implicit conversions from the input type to this type, but + * raise an exception if the input is null. */ - template * = nullptr> - explicit py_ref_tmpl(const py_ref_tmpl &other) noexcept + template * = nullptr> + py_ref_tmpl(const py_ref_tmpl &other) { - o = other.o; - incref_if_owned(o); + if (other.o != nullptr) { + o = other.o; + incref_if_owned(o); + } + else { + throw std::invalid_argument("Cannot convert null valued pointer to non-null reference."); + } } // Move constructors are really only useful for moving - // between types that own their references. + // from types that own their references. // Only define them for those cases. /* If: - * both this type and the input type own their reference, + * the input type owns its reference, * and we are not moving from a type that allows null values to one that does not, * Then: - * a move operation can be implicitly performed. + * a move operation is defined and will not raise an exception. */ - template * = nullptr> + template * = nullptr> py_ref_tmpl(py_ref_tmpl &&other) noexcept { + // If this type is a non-null type, the assigned value should not be null. + // If the other type is a non-null type, the provided value should not be null, + // unless it is being used uninitialized or after being moved from. + PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // Use get to assert that other.o is not null if other_not_null is true. o = other.o; + other.o = nullptr; + // The other type owns its reference. + // If this one does not, decref the new pointer. + // If this type is a non-null type, the assigned value should not be null. + decref_if_owned(o); + // Make sure other can be destructed without decrefing + // this object's wrapped pointer. } /* If: - * both this type and the input type own their reference, + * the input type owns its reference, * and we are moving from a type that allows null values to one that does not, * Then: - * only an explicit move operation can be performed. + * a move may be performed, but may also raise an exception. */ - template * = nullptr> - explicit py_ref_tmpl(py_ref_tmpl &&other) noexcept + template * = nullptr> + py_ref_tmpl(py_ref_tmpl &&other) { - o = other.o; + if (other.o != nullptr) { + o = other.o; + other.o = nullptr; + // The other type owns its reference. + // If this one does not, decref the new pointer. + // The assigned value is already known not be null. + decref_if_owned(o); + // Make sure other can be destructed without decrefing + // this object's wrapped pointer. + } + else { + throw std::invalid_argument("Cannot convert null valued pointer to non-null reference."); + } } /* When constructing from a PyObject*, the boolean value `consume_ref` must @@ -198,7 +241,13 @@ class py_ref_tmpl { } } - ~py_ref_tmpl() { decref_if_owned(o); } + ~py_ref_tmpl() + { + // A smart pointer wrapping a PyObject* still needs to be safe to + // destruct after it has been moved from. + // Because of that, always zero check when doing the last decref. + decref_if_owned(o); + } // For assignment operators, only allow assignment // in cases where implicit conversions are also allowed. @@ -213,58 +262,106 @@ class py_ref_tmpl { * this type doesn't allow null * and the input type doesn't allow null, * Then: - * Allow assignment from the other py_ref_tmpl type to this type. + * Assignment from the other py_ref_tmpl type to this type may not raise an exception. */ - template * = nullptr> - py_ref_tmpl operator=(const py_ref_tmpl &other) noexcept + py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept { - decref_if_owned(o); + PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // Nullcheck when doing decref in case this object + // is uninitialized or has been moved from. + decref_if_owned(o); o = other.o; incref_if_owned(o); + return *this; } - // Check if the wrapped pointer is null. - // Always return true if it is not null by definition. - bool is_null() noexcept + /* If: + * this type does not allow null, + * and the other type does, + * Then: + * Assignment from the other py_ref_tmpl type to this type may raise an exception. + */ + template * = nullptr> + py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept { - // This function will be a no-op returning true - // when not_null is false. - if (not_null || (o != nullptr)) { - return false; + if (other.o != nullptr) { + // Nullcheck when doing decref in case this object + // is uninitialized or has been moved from. + decref_if_owned(o); + o = other.o; + incref_if_owned(o); + return *this; + } + else { + throw std::invalid_argument("Cannot assign null valued pointer to non-null reference."); } - return true; } - // A debug version of is_null with a purely dynamic check. - bool is_null_dbg() noexcept { return o != nullptr; } -}; + // Same as previous two, except these assign from rvalues rather than lvalues. -// Default constructors for various cases. -template <> -inline py_ref_tmpl::py_ref_tmpl() noexcept -{ - o = Py_None; - incref(o); -} + /* If: + * this type allows null, + * Or: + * this type doesn't allow null + * and the input type doesn't allow null, + * Then: + * Assignment from the other py_ref_tmpl type to this type may not raise an exception. + */ + template * = nullptr> + py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept + { + PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // Nullcheck when doing decref in case this object + // is uninitialized or has been moved from. + decref_if_owned(o); + o = other.o; + other.o = nullptr; + incref_if_owned(o); + return *this; + } -template <> -inline py_ref_tmpl::py_ref_tmpl() noexcept -{ - o = nullptr; -} + /* If: + * this type does not allow null, + * and the other type does, + * Then: + * Assignment from the other py_ref_tmpl type to this type may raise an exception. + */ + template * = nullptr> + py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept + { + if (other.o != nullptr) { + // Nullcheck when doing decref in case this object + // is uninitialized or has been moved from. + decref_if_owned(o); + o = other.o; + other.o = nullptr; + incref_if_owned(o); + return *this; + } + else { + throw std::invalid_argument("Cannot assign null valued pointer to non-null reference."); + } + } -template <> -inline py_ref_tmpl::py_ref_tmpl() noexcept -{ - o = Py_None; -} + // Return an owned reference to the encapsulated PyObject as a raw pointer. + // Set the encapsulated pointer to NULL. + PyObject *release() noexcept + { + PYDYND_ASSERT_IF(not_null, o != nullptr); + auto ret = o; + o = nullptr; + incref_if_owned(ret); + return ret; + } -template <> -inline py_ref_tmpl::py_ref_tmpl() noexcept -{ - o = nullptr; -} + // Check if the wrapped pointer is null. + bool is_null() noexcept { return o != nullptr; } +}; // Convenience aliases for the templated smart pointer classes. @@ -276,33 +373,17 @@ using py_borref = py_ref_tmpl; using py_borref_with_null = py_ref_tmpl; -// To help with the transition to the new classes. -using pyobject_ownref = py_ref_tmpl; - -/* Check if a wrapped pointer is null. - * If it is not, return the pointer - * wrapped in the corresponding not_null wrapper type. - * If it is, raise an exception. - * This can be used to forward exceptions from Python. - */ -template -py_ref_tmpl check_null(py_ref_tmpl &o) -{ - if (o.is_null()) { - throw std::runtime_error("Unexpected null pointer."); - } - return reinterpret_cast>(o); -} - /* Capture a new reference if it is not null. * Throw an exception if it is. */ inline py_ref capture_if_not_null(PyObject *o) { if (o == nullptr) { - throw std::runtime_error("Unexpected null pouter."); + throw std::runtime_error("Unexpected null pointer."); } - return py_ref(o, true); + auto p = py_ref(o, true); + return p; + // return py_ref(o, true); } class PyGILState_RAII { diff --git a/dynd/nd/array.pyx b/dynd/nd/array.pyx index eb459ebd..7a372adb 100644 --- a/dynd/nd/array.pyx +++ b/dynd/nd/array.pyx @@ -25,6 +25,7 @@ from ..cpp.view cimport view as _view from ..pyobject_type cimport pyobject_id from ..config cimport translate_exception +from ..ndt import type as ndt_type from ..ndt.type cimport (type as _py_type, dynd_ndt_type_to_cpp, as_cpp_type, cpp_type_for, _register_nd_array_type_deduction) @@ -156,7 +157,6 @@ cdef class array(object): self.v = cpp_empty(dst_tp) self.v.assign(pyobject_array(value)) else: - from ..ndt import type as ndt_type if (not isinstance(type, ndt_type)): type = ndt_type(type) dst_tp = dynd_ndt_type_to_cpp(type) diff --git a/dynd/src/array_as_numpy.cpp b/dynd/src/array_as_numpy.cpp index 0185d3e6..7430bc1b 100644 --- a/dynd/src/array_as_numpy.cpp +++ b/dynd/src/array_as_numpy.cpp @@ -145,8 +145,8 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co make_numpy_dtype_for_copy(&child_numpy_dtype, 0, element_tp, arrmeta); // Create the result numpy dtype py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.get()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); PyArray_Descr *result = NULL; if (!PyArray_DescrConverter(tuple_obj.get(), &result)) { @@ -170,7 +170,7 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.get()); + PyList_SET_ITEM(names_obj.get(), i, name_str.release()); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); @@ -184,7 +184,7 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co size_t field_size = ((PyArray_Descr *)field_numpy_dtype.get())->elsize; standard_offset = inc_to_alignment(standard_offset, field_alignment); standard_alignment = max(standard_alignment, field_alignment); - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.get()); + PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(standard_offset)); standard_offset += field_size; } @@ -333,8 +333,8 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, } // Create the result numpy dtype py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.get()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); PyArray_Descr *result = NULL; if (!PyArray_DescrConverter(tuple_obj, &result)) { @@ -367,7 +367,7 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.get()); + PyList_SET_ITEM(names_obj.get(), i, name_str.release()); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); @@ -380,7 +380,7 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, *out_numpy_dtype = py_ref(Py_None, false); return; } - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.get()); + PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); } py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); @@ -528,7 +528,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) throw dynd::type_error(ss.str()); } } - return result.get(); + return result.release(); } if (a.get_type().get_id() == var_dim_id) { @@ -571,17 +571,17 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) } // Create a new NumPy array, and copy from the dynd array - py_ref result = capture_if_not_null(PyArray_NewFromDescr(&PyArray_Type, (PyArray_Descr *)numpy_dtype.get(), + py_ref result = capture_if_not_null(PyArray_NewFromDescr(&PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), (int)ndim, shape.get(), strides.get(), NULL, 0, NULL)); array_copy_to_numpy((PyArrayObject *)result.get(), a.get_type(), a.get()->metadata(), a.cdata()); // Return the NumPy array - return result.get(); + return result.release(); } else { // Create a view directly to the dynd array py_ref result = capture_if_not_null(PyArray_NewFromDescr( - &PyArray_Type, (PyArray_Descr *)numpy_dtype.get(), (int)ndim, shape.get(), strides.get(), + &PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), (int)ndim, shape.get(), strides.get(), const_cast(a.cdata()), ((a.get_flags() & nd::write_access_flag) ? NPY_ARRAY_WRITEABLE : 0) | NPY_ARRAY_ALIGNED, NULL)); @@ -594,7 +594,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) PyArray_BASE(result.get()) = n_obj; Py_INCREF(n_obj); #endif - return result.get(); + return result.release(); } } diff --git a/dynd/src/numpy_type_interop.cpp b/dynd/src/numpy_type_interop.cpp index 125875e6..5c2d5fec 100644 --- a/dynd/src/numpy_type_interop.cpp +++ b/dynd/src/numpy_type_interop.cpp @@ -116,7 +116,7 @@ ttp->get_field_types_raw(), offsets.data()); py_ref name_str = capture_if_not_null(PyString_FromStringAndSize( fname.begin, fname.end - fname.begin)); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.get()); + PyList_SET_ITEM(names_obj.get(), i, name_str.release()); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { @@ -175,8 +175,8 @@ dtype because it is not C-order"; py_ref shape_obj = capture_if_not_null(intptr_array_as_tuple((int)shape.size(), &shape[0])); py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, dtype_obj.get()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape_obj.get()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, dtype_obj.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 1, shape_obj.release()); PyArray_Descr *result = NULL; if (PyArray_DescrConverter(tuple_obj, &result) != NPY_SUCCEED) { throw dynd::type_error("failed to convert dynd type into numpy @@ -218,7 +218,7 @@ PyArray_Descr *pydynd::numpy_dtype_from__type(const dynd::ndt::type &tp, const c #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.get()); + PyList_SET_ITEM(names_obj.get(), i, name_str.release()); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); From ecc96d3584215b8c8d965baaf75e5f517cb3b654 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Mon, 6 Jun 2016 17:54:48 -0600 Subject: [PATCH 05/18] Add RAII class to release the GIL. Rename the existing RAII class for acquiring the GIL to something slightly less verbose. --- .../callables/apply_pyobject_callable.hpp | 58 +++++++------- .../include/kernels/apply_pyobject_kernel.hpp | 2 +- dynd/include/kernels/numpy_ufunc.hpp | 78 +++++++------------ dynd/include/utility_functions.hpp | 26 +++++-- 4 files changed, 79 insertions(+), 85 deletions(-) diff --git a/dynd/include/callables/apply_pyobject_callable.hpp b/dynd/include/callables/apply_pyobject_callable.hpp index c0bf8b77..0badcbc8 100644 --- a/dynd/include/callables/apply_pyobject_callable.hpp +++ b/dynd/include/callables/apply_pyobject_callable.hpp @@ -24,35 +24,35 @@ namespace nd { return dst_tp; } -/* - void instantiate(dynd::nd::call_node *&node, char *DYND_UNUSED(data), dynd::nd::kernel_builder *ckb, - const dynd::ndt::type &dst_tp, const char *dst_arrmeta, intptr_t nsrc, - const dynd::ndt::type *src_tp, const char *const *src_arrmeta, dynd::kernel_request_t kernreq, - intptr_t nkwd, const dynd::nd::array *kwds, - const std::map &tp_vars) - { - pydynd::PyGILState_RAII pgs; - - std::vector src_tp_copy(nsrc); - for (int i = 0; i < nsrc; ++i) { - src_tp_copy[i] = src_tp[i]; - } - - intptr_t ckb_offset = ckb->size(); - ckb->emplace_back(kernreq); - apply_pyobject_kernel *self = ckb->get_at(ckb_offset); - self->m_proto = dynd::ndt::callable_type::make(dst_tp, src_tp_copy); - self->m_pyfunc = func; - Py_XINCREF(self->m_pyfunc); - self->m_dst_arrmeta = dst_arrmeta; - self->m_src_arrmeta.resize(nsrc); - copy(src_arrmeta, src_arrmeta + nsrc, self->m_src_arrmeta.begin()); - - dynd::ndt::type child_src_tp = dynd::ndt::make_type(); - dynd::nd::assign->instantiate(node, nullptr, ckb, dst_tp, dst_arrmeta, 1, &child_src_tp, nullptr, - dynd::kernel_request_single, 0, nullptr, tp_vars); - } -*/ + /* + void instantiate(dynd::nd::call_node *&node, char *DYND_UNUSED(data), dynd::nd::kernel_builder *ckb, + const dynd::ndt::type &dst_tp, const char *dst_arrmeta, intptr_t nsrc, + const dynd::ndt::type *src_tp, const char *const *src_arrmeta, dynd::kernel_request_t kernreq, + intptr_t nkwd, const dynd::nd::array *kwds, + const std::map &tp_vars) + { + pydynd::with_gil pgs; + + std::vector src_tp_copy(nsrc); + for (int i = 0; i < nsrc; ++i) { + src_tp_copy[i] = src_tp[i]; + } + + intptr_t ckb_offset = ckb->size(); + ckb->emplace_back(kernreq); + apply_pyobject_kernel *self = ckb->get_at(ckb_offset); + self->m_proto = dynd::ndt::callable_type::make(dst_tp, src_tp_copy); + self->m_pyfunc = func; + Py_XINCREF(self->m_pyfunc); + self->m_dst_arrmeta = dst_arrmeta; + self->m_src_arrmeta.resize(nsrc); + copy(src_arrmeta, src_arrmeta + nsrc, self->m_src_arrmeta.begin()); + + dynd::ndt::type child_src_tp = dynd::ndt::make_type(); + dynd::nd::assign->instantiate(node, nullptr, ckb, dst_tp, dst_arrmeta, 1, &child_src_tp, nullptr, + dynd::kernel_request_single, 0, nullptr, tp_vars); + } + */ }; } // namespace pydynd::nd::functional diff --git a/dynd/include/kernels/apply_pyobject_kernel.hpp b/dynd/include/kernels/apply_pyobject_kernel.hpp index f37fc795..31a44295 100644 --- a/dynd/include/kernels/apply_pyobject_kernel.hpp +++ b/dynd/include/kernels/apply_pyobject_kernel.hpp @@ -22,7 +22,7 @@ struct apply_pyobject_kernel : dynd::nd::base_strided_kerneldestroy(); } diff --git a/dynd/include/kernels/numpy_ufunc.hpp b/dynd/include/kernels/numpy_ufunc.hpp index 565f2d49..ed014ddf 100644 --- a/dynd/include/kernels/numpy_ufunc.hpp +++ b/dynd/include/kernels/numpy_ufunc.hpp @@ -16,7 +16,7 @@ namespace nd { { if (ufunc != NULL) { // Acquire the GIL for the python decref - PyGILState_RAII pgs; + with_gil pgs; Py_DECREF(ufunc); } } @@ -26,8 +26,7 @@ namespace nd { struct scalar_ufunc_ck; template <> - struct scalar_ufunc_ck - : dynd::nd::base_strided_kernel, 1> { + struct scalar_ufunc_ck : dynd::nd::base_strided_kernel, 1> { typedef scalar_ufunc_ck self_type; const scalar_ufunc_data *data; @@ -47,8 +46,7 @@ namespace nd { data->funcptr(args, &dimsize, strides, data->ufunc_data); } - void strided(char *dst, intptr_t dst_stride, char *const *src, - const intptr_t *src_stride, size_t count) + void strided(char *dst, intptr_t dst_stride, char *const *src, const intptr_t *src_stride, size_t count) { char *args[NPY_MAXARGS]; // Set up the args array the way the numpy ufunc wants it @@ -56,37 +54,26 @@ namespace nd { args[data->param_count] = dst; // Call the ufunc loop with a dim size of 1 intptr_t strides[NPY_MAXARGS]; - memcpy(&strides[0], &src_stride[0], - data->param_count * sizeof(intptr_t)); + memcpy(&strides[0], &src_stride[0], data->param_count * sizeof(intptr_t)); strides[data->param_count] = dst_stride; - data->funcptr(args, reinterpret_cast(&count), strides, - data->ufunc_data); + data->funcptr(args, reinterpret_cast(&count), strides, data->ufunc_data); } - static void - instantiate(char *static_data, char *DYND_UNUSED(data), - kernel_builder *ckb, intptr_t ckb_offset, - const dynd::ndt::type &dst_tp, - const char *DYND_UNUSED(dst_arrmeta), - intptr_t DYND_UNUSED(nsrc), const dynd::ndt::type *src_tp, - const char *const *DYND_UNUSED(src_arrmeta), - dynd::kernel_request_t kernreq, - const dynd::eval::eval_context *DYND_UNUSED(ectx), - intptr_t nkwd, const dynd::nd::array *kwds, - const std::map &tp_vars) + static void instantiate(char *static_data, char *DYND_UNUSED(data), kernel_builder *ckb, intptr_t ckb_offset, + const dynd::ndt::type &dst_tp, const char *DYND_UNUSED(dst_arrmeta), + intptr_t DYND_UNUSED(nsrc), const dynd::ndt::type *src_tp, + const char *const *DYND_UNUSED(src_arrmeta), dynd::kernel_request_t kernreq, + const dynd::eval::eval_context *DYND_UNUSED(ectx), intptr_t nkwd, + const dynd::nd::array *kwds, const std::map &tp_vars) { // Acquire the GIL for creating the ckernel - PyGILState_RAII pgs; - self_type::make( - ckb, kernreq, - reinterpret_cast *>(static_data) - ->get()); + with_gil pgs; + self_type::make(ckb, kernreq, reinterpret_cast *>(static_data)->get()); } }; template <> - struct scalar_ufunc_ck - : dynd::nd::base_strided_kernel, 1> { + struct scalar_ufunc_ck : dynd::nd::base_strided_kernel, 1> { typedef scalar_ufunc_ck self_type; const scalar_ufunc_data *data; @@ -104,13 +91,12 @@ namespace nd { intptr_t strides[NPY_MAXARGS]; memset(strides, 0, (data->param_count + 1) * sizeof(void *)); { - PyGILState_RAII pgs; + with_gil pgs; data->funcptr(args, &dimsize, strides, data->ufunc_data); } } - void strided(char *dst, intptr_t dst_stride, char *const *src, - const intptr_t *src_stride, size_t count) + void strided(char *dst, intptr_t dst_stride, char *const *src, const intptr_t *src_stride, size_t count) { char *args[NPY_MAXARGS]; // Set up the args array the way the numpy ufunc wants it @@ -118,33 +104,25 @@ namespace nd { args[data->param_count] = dst; // Call the ufunc loop with a dim size of 1 intptr_t strides[NPY_MAXARGS]; - memcpy(&strides[0], &src_stride[0], - data->param_count * sizeof(intptr_t)); + memcpy(&strides[0], &src_stride[0], data->param_count * sizeof(intptr_t)); strides[data->param_count] = dst_stride; { - PyGILState_RAII pgs; - data->funcptr(args, reinterpret_cast(&count), strides, - data->ufunc_data); + with_gil pgs; + data->funcptr(args, reinterpret_cast(&count), strides, data->ufunc_data); } } - static intptr_t - instantiate(char *static_data, char *DYND_UNUSED(data), void *ckb, - intptr_t ckb_offset, const dynd::ndt::type &dst_tp, - const char *DYND_UNUSED(dst_arrmeta), - intptr_t DYND_UNUSED(nsrc), const dynd::ndt::type *src_tp, - const char *const *DYND_UNUSED(src_arrmeta), - dynd::kernel_request_t kernreq, - const dynd::eval::eval_context *DYND_UNUSED(ectx), - intptr_t nkwd, const dynd::nd::array *kwds, - const std::map &tp_vars) + static intptr_t instantiate(char *static_data, char *DYND_UNUSED(data), void *ckb, intptr_t ckb_offset, + const dynd::ndt::type &dst_tp, const char *DYND_UNUSED(dst_arrmeta), + intptr_t DYND_UNUSED(nsrc), const dynd::ndt::type *src_tp, + const char *const *DYND_UNUSED(src_arrmeta), dynd::kernel_request_t kernreq, + const dynd::eval::eval_context *DYND_UNUSED(ectx), intptr_t nkwd, + const dynd::nd::array *kwds, const std::map &tp_vars) { // Acquire the GIL for creating the ckernel - PyGILState_RAII pgs; - self_type::make( - ckb, kernreq, ckb_offset, - reinterpret_cast *>(static_data) - ->get()); + with_gil pgs; + self_type::make(ckb, kernreq, ckb_offset, + reinterpret_cast *>(static_data)->get()); return ckb_offset; } }; diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index ba77a7b3..5e5c112b 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -386,16 +386,32 @@ inline py_ref capture_if_not_null(PyObject *o) // return py_ref(o, true); } -class PyGILState_RAII { +// RAII class to acquire GIL. +class with_gil { PyGILState_STATE m_gstate; - PyGILState_RAII(const PyGILState_RAII &); - PyGILState_RAII &operator=(const PyGILState_RAII &); + with_gil(const with_gil &) = delete; + + with_gil &operator=(const with_gil &) = delete; + +public: + inline with_gil() { m_gstate = PyGILState_Ensure(); } + + inline ~with_gil() { PyGILState_Release(m_gstate); } +}; + +// RAII class to release GIL. +class with_nogil { + PyThreadState *m_tstate; + + with_nogil(const with_nogil &) = delete; + + with_nogil &operator=(const with_nogil &) = delete; public: - inline PyGILState_RAII() { m_gstate = PyGILState_Ensure(); } + inline with_nogil() { m_tstate = PyEval_SaveThread(); } - inline ~PyGILState_RAII() { PyGILState_Release(m_gstate); } + inline ~with_nogil() { PyEval_RestoreThread(m_tstate); } }; /** From fb7d0eb0d6b5299324d8221355a92697d35720f0 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 13:48:41 -0600 Subject: [PATCH 06/18] Add explicit syntax for borrowing a reference from a wrapped PyObject*. --- dynd/include/utility_functions.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 5e5c112b..669bc456 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -361,6 +361,8 @@ class py_ref_tmpl { // Check if the wrapped pointer is null. bool is_null() noexcept { return o != nullptr; } + + py_ref_tmpl borrow() noexcept { return py_ref(o, false); } }; // Convenience aliases for the templated smart pointer classes. From 791696a54ebb753a9ede9ad8aaefd25c39d30287 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 14:37:19 -0600 Subject: [PATCH 07/18] Add convenience function for null-checking a smart pointer wrapping a PyObject*. --- dynd/include/utility_functions.hpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 669bc456..d0933e66 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -388,6 +388,19 @@ inline py_ref capture_if_not_null(PyObject *o) // return py_ref(o, true); } +/* Convert to a non-null reference. + * If the input type allows nulls, explicitly check for null and raise an exception. + * If the input type does not allow null, this function is marked as noexcept and + * only checks for via an assert statement in debug builds. + */ +template +inline py_ref_tmpl nullcheck(py_ref_tmpl &&obj) noexcept(not_null) +{ + // Route this through the assignment operator since the semantics are the same. + py_ref_tmpl out = obj; + return out; +} + // RAII class to acquire GIL. class with_gil { PyGILState_STATE m_gstate; From 24d4a320e6d4ce6cfea5aac3814dc4fc932918f6 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 14:54:09 -0600 Subject: [PATCH 08/18] Add a convenience function to aid in casting from a possibly null reference to a reference that is known not to be null. --- dynd/include/utility_functions.hpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index d0933e66..d6dc1de4 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -401,6 +401,17 @@ inline py_ref_tmpl nullcheck(py_ref_tmpl &&o return out; } +/* Convert to a non-null reference. + * No nullcheck is used, except for an assertion in debug builds. + * This should be used when the pointer is already known to not be null. + */ +template +inline py_ref_tmpl disallow_null(py_ref_tmpl &&obj) noexcept +{ + assert(obj.get() != nullptr); + return py_ref_tmpl(obj.release(), owns_ref); +} + // RAII class to acquire GIL. class with_gil { PyGILState_STATE m_gstate; From 4d0d38eb297f02f052ca24c1c1f5d3c57915e876 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 16:20:50 -0600 Subject: [PATCH 09/18] Add a series of assert statements to ensure that wrapped references are always valid. --- dynd/include/utility_functions.hpp | 87 +++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 25 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index d6dc1de4..9d7c74b3 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -47,6 +47,8 @@ inline void decref(PyObject *) noexcept; template <> inline void decref(PyObject *obj) noexcept { + // Assert that the reference is valid if it is not null. + PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); Py_XDECREF(obj); } @@ -54,6 +56,8 @@ template <> inline void decref(PyObject *obj) noexcept { assert(obj != nullptr); + // Assert that the reference is valid. + assert(Py_REFCNT(obj) > 0); Py_DECREF(obj); } @@ -90,23 +94,33 @@ template <> inline void decref_if_owned(PyObject *obj) noexcept { assert(obj != nullptr); + // Assert that the reference is valid. + assert(Py_REFCNT(obj) > 0); Py_DECREF(obj); } template <> inline void decref_if_owned(PyObject *obj) noexcept { + // Assert that the reference is valid if it is not null. + PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); Py_XDECREF(obj); } +// This is a no-op in non-debug builds. +// In debug builds, it asserts that the reference is actually valid. template <> inline void decref_if_owned(PyObject *obj) noexcept { + assert(Py_REFCNT(obj) > 0); } +// This is a no-op in non-debug builds. +// In debug builds, it asserts that the reference is actually valid. template <> inline void decref_if_owned(PyObject *obj) noexcept { + PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); } template @@ -125,6 +139,8 @@ class py_ref_tmpl { PyObject *get() noexcept { PYDYND_ASSERT_IF(not_null, o != nullptr); + // Assert that the accessed reference is still valid. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); return o; } @@ -133,18 +149,18 @@ class py_ref_tmpl { // they are also declared noexcept. /* If: - * this type allows null, - * Or: - * this type doesn't allow null - * and the input type doesn't allow null, + * This type allows null or the input type does not, * Then: * Conversions from the other py_ref_tmpl type to this type do not raise exceptions. */ template * = nullptr> + typename std::enable_if_t * = nullptr> py_ref_tmpl(const py_ref_tmpl &other) noexcept { - PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // If the input is not null, assert that it isn't. + PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); + // Assert that the input reference is valid if it is not null. + PYDYND_ASSERT_IF(other.o != nullptr, Py_REFCNT(other.o) > 0); o = other.o; incref_if_owned(o); } @@ -162,6 +178,8 @@ class py_ref_tmpl { py_ref_tmpl(const py_ref_tmpl &other) { if (other.o != nullptr) { + // Assert that the input reference is valid. + assert(Py_REFCNT(other.o) > 0); o = other.o; incref_if_owned(o); } @@ -180,22 +198,26 @@ class py_ref_tmpl { * Then: * a move operation is defined and will not raise an exception. */ - template * = nullptr> + template * = nullptr> py_ref_tmpl(py_ref_tmpl &&other) noexcept { // If this type is a non-null type, the assigned value should not be null. // If the other type is a non-null type, the provided value should not be null, // unless it is being used uninitialized or after being moved from. PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // If the reference is not null, assert that it is valid. + PYDYND_ASSERT_IF(other.o != nullptr, Py_REFCNT(other.o) > 0); // Use get to assert that other.o is not null if other_not_null is true. o = other.o; + // Make sure other can be destructed without decrefing + // this object's wrapped pointer. other.o = nullptr; // The other type owns its reference. // If this one does not, decref the new pointer. // If this type is a non-null type, the assigned value should not be null. decref_if_owned(o); - // Make sure other can be destructed without decrefing - // this object's wrapped pointer. + // If the reference is not null, assert that it is still valid. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0) } /* If: @@ -208,14 +230,18 @@ class py_ref_tmpl { py_ref_tmpl(py_ref_tmpl &&other) { if (other.o != nullptr) { + // Assert that the input reference is valid. + assert(Py_REFCNT(other.o) > 0); o = other.o; + // Make sure other can be destructed without decrefing + // this object's wrapped pointer. other.o = nullptr; // The other type owns its reference. // If this one does not, decref the new pointer. // The assigned value is already known not be null. decref_if_owned(o); - // Make sure other can be destructed without decrefing - // this object's wrapped pointer. + // Assert that the stored reference is still valid + assert(Py_REFCNT(o) > 0); } else { throw std::invalid_argument("Cannot convert null valued pointer to non-null reference."); @@ -239,6 +265,9 @@ class py_ref_tmpl { else { incref_if_owned(o); } + // Regardless of whether or not a reference was consumed, + // assert that it is valid if it is not null. + PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); } ~py_ref_tmpl() @@ -257,18 +286,18 @@ class py_ref_tmpl { // Assignment never comsumes a reference. /* If: - * this type allows null, - * Or: - * this type doesn't allow null - * and the input type doesn't allow null, + * This type allows null or the input type does not, * Then: * Assignment from the other py_ref_tmpl type to this type may not raise an exception. */ template * = nullptr> + typename std::enable_if_t * = nullptr> py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept { - PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // Assert that the input reference is not null if the input type does not allow nulls. + PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); + // Assert that the input reference is valid if it is not null. + PYDYND_ASSERT_IF(other.o != nullptr, Py_REFCNT(other.o) > 0); // Nullcheck when doing decref in case this object // is uninitialized or has been moved from. decref_if_owned(o); @@ -288,6 +317,8 @@ class py_ref_tmpl { py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept { if (other.o != nullptr) { + // Assert that the input reference is valid. + assert(Py_REFCNT(other.o) > 0); // Nullcheck when doing decref in case this object // is uninitialized or has been moved from. decref_if_owned(o); @@ -311,10 +342,13 @@ class py_ref_tmpl { * Assignment from the other py_ref_tmpl type to this type may not raise an exception. */ template * = nullptr> + typename std::enable_if_t * = nullptr> py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept { - PYDYND_ASSERT_IF(not_null || other_not_null, other.o != nullptr); + // If the input reference should not be null, assert that that is the case. + PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); + // If the input reference is not null, assert that it is valid. + PYDYND_ASSERT_IF(other.o != nullptr, Py_REFCNT(other.o) > 0); // Nullcheck when doing decref in case this object // is uninitialized or has been moved from. decref_if_owned(o); @@ -335,6 +369,8 @@ class py_ref_tmpl { py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept { if (other.o != nullptr) { + // Assert that the input reference is valid. + assert(Py_REFCNT(other.o) > 0); // Nullcheck when doing decref in case this object // is uninitialized or has been moved from. decref_if_owned(o); @@ -352,16 +388,16 @@ class py_ref_tmpl { // Set the encapsulated pointer to NULL. PyObject *release() noexcept { + // If the contained reference should not be null, assert that it isn't. PYDYND_ASSERT_IF(not_null, o != nullptr); + // If the contained reference is not null, assert that it is valid. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); auto ret = o; o = nullptr; incref_if_owned(ret); return ret; } - // Check if the wrapped pointer is null. - bool is_null() noexcept { return o != nullptr; } - py_ref_tmpl borrow() noexcept { return py_ref(o, false); } }; @@ -383,9 +419,7 @@ inline py_ref capture_if_not_null(PyObject *o) if (o == nullptr) { throw std::runtime_error("Unexpected null pointer."); } - auto p = py_ref(o, true); - return p; - // return py_ref(o, true); + return py_ref(o, true); } /* Convert to a non-null reference. @@ -408,7 +442,10 @@ inline py_ref_tmpl nullcheck(py_ref_tmpl &&o template inline py_ref_tmpl disallow_null(py_ref_tmpl &&obj) noexcept { + // Assert that the wrapped pointer is actually not null. assert(obj.get() != nullptr); + // Assert that the wrapped reference is valid if it is not null. + PYDYND_ASSERT_IF(obj.get() != nullptr, Py_REFCNT(obj.get()) > 0); return py_ref_tmpl(obj.release(), owns_ref); } From ebeb8839e08aca2710ad2ced81166d9050f345c8 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 16:25:06 -0600 Subject: [PATCH 10/18] Make release method for py_ref return an owned or borrowed reference depending on the type of reference the smart pointer wraps. --- dynd/include/utility_functions.hpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 9d7c74b3..1697bce6 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -384,8 +384,10 @@ class py_ref_tmpl { } } - // Return an owned reference to the encapsulated PyObject as a raw pointer. + // Return a reference to the encapsulated PyObject as a raw pointer. // Set the encapsulated pointer to NULL. + // If this is a type that owns its reference, an owned reference is returned. + // If this is a type that wraps a borrowed reference, a borrowed reference is returned. PyObject *release() noexcept { // If the contained reference should not be null, assert that it isn't. @@ -394,7 +396,6 @@ class py_ref_tmpl { PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); auto ret = o; o = nullptr; - incref_if_owned(ret); return ret; } From 059e012ba7b5fb3e1af378678aae8909ed469800 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 17:25:33 -0600 Subject: [PATCH 11/18] Move the release method of pydynd::py_ref_tmpl to be a function that takes an rvalue reference so that its inputs are explicitly invalidated when release is used. --- .../callables/assign_to_pyobject_callable.hpp | 4 +-- .../kernels/assign_to_pyobject_kernel.hpp | 8 ++--- dynd/include/utility_functions.hpp | 22 ++++++++----- dynd/src/array_as_numpy.cpp | 31 ++++++++++--------- dynd/src/numpy_type_interop.cpp | 8 ++--- 5 files changed, 40 insertions(+), 33 deletions(-) diff --git a/dynd/include/callables/assign_to_pyobject_callable.hpp b/dynd/include/callables/assign_to_pyobject_callable.hpp index f259ba15..98a85d8c 100644 --- a/dynd/include/callables/assign_to_pyobject_callable.hpp +++ b/dynd/include/callables/assign_to_pyobject_callable.hpp @@ -294,7 +294,7 @@ namespace nd { const dynd::string &rawname = src0_tp.extended()->get_field_name(i); pydynd::py_ref name = capture_if_not_null(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); - PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.release()); + PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, pydynd::release(std::move(name))); } self_ck->m_copy_el_offsets.resize(field_count); @@ -339,7 +339,7 @@ namespace nd { const dynd::string &rawname = src_tp[0].extended()->get_field_name(i); pydynd::py_ref name = capture_if_not_null(PyUnicode_DecodeUTF8(rawname.begin(), rawname.end() - rawname.begin(), NULL)); - PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, name.release()); + PyTuple_SET_ITEM(self_ck->m_field_names.get(), i, pydynd::release(std::move(name))); } self_ck->m_copy_el_offsets.resize(field_count); for (intptr_t i = 0; i < field_count; ++i) { diff --git a/dynd/include/kernels/assign_to_pyobject_kernel.hpp b/dynd/include/kernels/assign_to_pyobject_kernel.hpp index 4ca5e992..5c820dc5 100644 --- a/dynd/include/kernels/assign_to_pyobject_kernel.hpp +++ b/dynd/include/kernels/assign_to_pyobject_kernel.hpp @@ -431,7 +431,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = tup.release(); + *dst_obj = pydynd::release(std::move(tup)); } }; @@ -470,7 +470,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = dct.release(); + *dst_obj = pydynd::release(std::move(dct)); } }; @@ -496,7 +496,7 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.release(); + *dst_obj = pydynd::release(std::move(lst)); } }; @@ -524,6 +524,6 @@ struct assign_to_pyobject_kernel if (PyErr_Occurred()) { throw std::exception(); } - *dst_obj = lst.release(); + *dst_obj = pydynd::release(std::move(lst)); } }; diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 1697bce6..7940460d 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -217,7 +217,7 @@ class py_ref_tmpl { // If this type is a non-null type, the assigned value should not be null. decref_if_owned(o); // If the reference is not null, assert that it is still valid. - PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0) + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); } /* If: @@ -384,22 +384,22 @@ class py_ref_tmpl { } } + py_ref_tmpl borrow() noexcept { return py_ref(o, false); } + // Return a reference to the encapsulated PyObject as a raw pointer. // Set the encapsulated pointer to NULL. // If this is a type that owns its reference, an owned reference is returned. // If this is a type that wraps a borrowed reference, a borrowed reference is returned. - PyObject *release() noexcept + static PyObject *release(py_ref_tmpl &&ref) noexcept { // If the contained reference should not be null, assert that it isn't. - PYDYND_ASSERT_IF(not_null, o != nullptr); + PYDYND_ASSERT_IF(not_null, ref.o != nullptr); // If the contained reference is not null, assert that it is valid. - PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); - auto ret = o; - o = nullptr; + PYDYND_ASSERT_IF(ref.o != nullptr, Py_REFCNT(ref.o) > 0); + PyObject *ret = ref.o; + ref.o = nullptr; return ret; } - - py_ref_tmpl borrow() noexcept { return py_ref(o, false); } }; // Convenience aliases for the templated smart pointer classes. @@ -412,6 +412,12 @@ using py_borref = py_ref_tmpl; using py_borref_with_null = py_ref_tmpl; +template +PyObject *release(py_ref_tmpl &&ref) +{ + return py_ref_tmpl::release(std::forward>(ref)); +} + /* Capture a new reference if it is not null. * Throw an exception if it is. */ diff --git a/dynd/src/array_as_numpy.cpp b/dynd/src/array_as_numpy.cpp index 7430bc1b..6ed941eb 100644 --- a/dynd/src/array_as_numpy.cpp +++ b/dynd/src/array_as_numpy.cpp @@ -145,8 +145,8 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co make_numpy_dtype_for_copy(&child_numpy_dtype, 0, element_tp, arrmeta); // Create the result numpy dtype py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, release(std::move(child_numpy_dtype))); + PyTuple_SET_ITEM(tuple_obj.get(), 1, release(std::move(shape))); PyArray_Descr *result = NULL; if (!PyArray_DescrConverter(tuple_obj.get(), &result)) { @@ -170,7 +170,7 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, release(std::move(name_str))); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); @@ -184,7 +184,7 @@ static void make_numpy_dtype_for_copy(py_ref *out_numpy_dtype, intptr_t ndim, co size_t field_size = ((PyArray_Descr *)field_numpy_dtype.get())->elsize; standard_offset = inc_to_alignment(standard_offset, field_alignment); standard_alignment = max(standard_alignment, field_alignment); - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); + PyList_SET_ITEM(formats_obj.get(), i, release(std::move(field_numpy_dtype))); PyList_SET_ITEM(offsets_obj.get(), i, PyLong_FromSize_t(standard_offset)); standard_offset += field_size; } @@ -333,8 +333,8 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, } // Create the result numpy dtype py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, child_numpy_dtype.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, release(std::move(child_numpy_dtype))); + PyTuple_SET_ITEM(tuple_obj.get(), 1, release(std::move(shape))); PyArray_Descr *result = NULL; if (!PyArray_DescrConverter(tuple_obj, &result)) { @@ -367,7 +367,7 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fn.begin(), fn.end() - fn.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, release(std::move(name_str))); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); @@ -380,7 +380,7 @@ static void as_numpy_analysis(py_ref *out_numpy_dtype, bool *out_requires_copy, *out_numpy_dtype = py_ref(Py_None, false); return; } - PyList_SET_ITEM(formats_obj.get(), i, field_numpy_dtype.release()); + PyList_SET_ITEM(formats_obj.get(), i, release(std::move(field_numpy_dtype))); } py_ref offsets_obj = capture_if_not_null(PyList_New(field_count)); @@ -528,7 +528,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) throw dynd::type_error(ss.str()); } } - return result.release(); + return release(std::move(result)); } if (a.get_type().get_id() == var_dim_id) { @@ -571,18 +571,19 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) } // Create a new NumPy array, and copy from the dynd array - py_ref result = capture_if_not_null(PyArray_NewFromDescr(&PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), - (int)ndim, shape.get(), strides.get(), NULL, 0, NULL)); + py_ref result = capture_if_not_null( + PyArray_NewFromDescr(&PyArray_Type, reinterpret_cast(release(std::move(numpy_dtype))), + (int)ndim, shape.get(), strides.get(), NULL, 0, NULL)); array_copy_to_numpy((PyArrayObject *)result.get(), a.get_type(), a.get()->metadata(), a.cdata()); // Return the NumPy array - return result.release(); + return release(std::move(result)); } else { // Create a view directly to the dynd array py_ref result = capture_if_not_null(PyArray_NewFromDescr( - &PyArray_Type, (PyArray_Descr *)numpy_dtype.release(), (int)ndim, shape.get(), strides.get(), - const_cast(a.cdata()), + &PyArray_Type, reinterpret_cast(release(std::move(numpy_dtype))), (int)ndim, shape.get(), + strides.get(), const_cast(a.cdata()), ((a.get_flags() & nd::write_access_flag) ? NPY_ARRAY_WRITEABLE : 0) | NPY_ARRAY_ALIGNED, NULL)); #if NPY_API_VERSION >= 7 // At least NumPy 1.7 @@ -594,7 +595,7 @@ PyObject *pydynd::array_as_numpy(PyObject *a_obj, bool allow_copy) PyArray_BASE(result.get()) = n_obj; Py_INCREF(n_obj); #endif - return result.release(); + return release(std::move(result)); } } diff --git a/dynd/src/numpy_type_interop.cpp b/dynd/src/numpy_type_interop.cpp index 5c2d5fec..c035fd71 100644 --- a/dynd/src/numpy_type_interop.cpp +++ b/dynd/src/numpy_type_interop.cpp @@ -116,7 +116,7 @@ ttp->get_field_types_raw(), offsets.data()); py_ref name_str = capture_if_not_null(PyString_FromStringAndSize( fname.begin, fname.end - fname.begin)); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, release(std::move(name_str))); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); for (size_t i = 0; i < field_count; ++i) { @@ -175,8 +175,8 @@ dtype because it is not C-order"; py_ref shape_obj = capture_if_not_null(intptr_array_as_tuple((int)shape.size(), &shape[0])); py_ref tuple_obj = capture_if_not_null(PyTuple_New(2)); - PyTuple_SET_ITEM(tuple_obj.get(), 0, dtype_obj.release()); - PyTuple_SET_ITEM(tuple_obj.get(), 1, shape_obj.release()); + PyTuple_SET_ITEM(tuple_obj.get(), 0, release(std::move(dtype_obj))); + PyTuple_SET_ITEM(tuple_obj.get(), 1, release(std::move(shape_obj))); PyArray_Descr *result = NULL; if (PyArray_DescrConverter(tuple_obj, &result) != NPY_SUCCEED) { throw dynd::type_error("failed to convert dynd type into numpy @@ -218,7 +218,7 @@ PyArray_Descr *pydynd::numpy_dtype_from__type(const dynd::ndt::type &tp, const c #else py_ref name_str = capture_if_not_null(PyString_FromStringAndSize(fname.begin(), fname.end() - fname.begin())); #endif - PyList_SET_ITEM(names_obj.get(), i, name_str.release()); + PyList_SET_ITEM(names_obj.get(), i, release(std::move(name_str))); } py_ref formats_obj = capture_if_not_null(PyList_New(field_count)); From 7f2eaba4988284ffd7bf3325ef147f31250c59ec Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 17:29:48 -0600 Subject: [PATCH 12/18] Rename py_ref_tmpl to py_ref_t. --- dynd/include/utility_functions.hpp | 58 +++++++++++++++--------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 7940460d..a463b9cf 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -124,7 +124,7 @@ inline void decref_if_owned(PyObject *obj) noexcept } template -class py_ref_tmpl { +class py_ref_t { PyObject *o; public: @@ -132,7 +132,7 @@ class py_ref_tmpl { // This allows the smart pointer class to be moved from. // Whether or not the class is allowed to be null only changes // when zerochecks/exceptions may occur and when assertions are used. - py_ref_tmpl() noexcept : o(nullptr){}; + py_ref_t() noexcept : o(nullptr){}; // First define an accessor to get the PyObject pointer from // the wrapper class. @@ -151,11 +151,11 @@ class py_ref_tmpl { /* If: * This type allows null or the input type does not, * Then: - * Conversions from the other py_ref_tmpl type to this type do not raise exceptions. + * Conversions from the other py_ref_t type to this type do not raise exceptions. */ template * = nullptr> - py_ref_tmpl(const py_ref_tmpl &other) noexcept + py_ref_t(const py_ref_t &other) noexcept { // If the input is not null, assert that it isn't. PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); @@ -175,7 +175,7 @@ class py_ref_tmpl { */ template * = nullptr> - py_ref_tmpl(const py_ref_tmpl &other) + py_ref_t(const py_ref_t &other) { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -199,7 +199,7 @@ class py_ref_tmpl { * a move operation is defined and will not raise an exception. */ template * = nullptr> - py_ref_tmpl(py_ref_tmpl &&other) noexcept + py_ref_t(py_ref_t &&other) noexcept { // If this type is a non-null type, the assigned value should not be null. // If the other type is a non-null type, the provided value should not be null, @@ -227,7 +227,7 @@ class py_ref_tmpl { * a move may be performed, but may also raise an exception. */ template * = nullptr> - py_ref_tmpl(py_ref_tmpl &&other) + py_ref_t(py_ref_t &&other) { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -256,7 +256,7 @@ class py_ref_tmpl { * specify `consume_ref` as false regardless of whether or not you * own the reference represented in the PyObject* you pass in. */ - explicit py_ref_tmpl(PyObject *obj, bool consume_ref) noexcept + explicit py_ref_t(PyObject *obj, bool consume_ref) noexcept { o = obj; if (consume_ref) { @@ -270,7 +270,7 @@ class py_ref_tmpl { PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); } - ~py_ref_tmpl() + ~py_ref_t() { // A smart pointer wrapping a PyObject* still needs to be safe to // destruct after it has been moved from. @@ -288,11 +288,11 @@ class py_ref_tmpl { /* If: * This type allows null or the input type does not, * Then: - * Assignment from the other py_ref_tmpl type to this type may not raise an exception. + * Assignment from the other py_ref_t type to this type may not raise an exception. */ template * = nullptr> - py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept + py_ref_t &operator=(const py_ref_t &other) noexcept { // Assert that the input reference is not null if the input type does not allow nulls. PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); @@ -310,11 +310,11 @@ class py_ref_tmpl { * this type does not allow null, * and the other type does, * Then: - * Assignment from the other py_ref_tmpl type to this type may raise an exception. + * Assignment from the other py_ref_t type to this type may raise an exception. */ template * = nullptr> - py_ref_tmpl &operator=(const py_ref_tmpl &other) noexcept + py_ref_t &operator=(const py_ref_t &other) noexcept { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -339,11 +339,11 @@ class py_ref_tmpl { * this type doesn't allow null * and the input type doesn't allow null, * Then: - * Assignment from the other py_ref_tmpl type to this type may not raise an exception. + * Assignment from the other py_ref_t type to this type may not raise an exception. */ template * = nullptr> - py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept + py_ref_t &operator=(py_ref_t &&other) noexcept { // If the input reference should not be null, assert that that is the case. PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); @@ -362,11 +362,11 @@ class py_ref_tmpl { * this type does not allow null, * and the other type does, * Then: - * Assignment from the other py_ref_tmpl type to this type may raise an exception. + * Assignment from the other py_ref_t type to this type may raise an exception. */ template * = nullptr> - py_ref_tmpl &operator=(py_ref_tmpl &&other) noexcept + py_ref_t &operator=(py_ref_t &&other) noexcept { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -384,13 +384,13 @@ class py_ref_tmpl { } } - py_ref_tmpl borrow() noexcept { return py_ref(o, false); } + py_ref_t borrow() noexcept { return py_ref(o, false); } // Return a reference to the encapsulated PyObject as a raw pointer. // Set the encapsulated pointer to NULL. // If this is a type that owns its reference, an owned reference is returned. // If this is a type that wraps a borrowed reference, a borrowed reference is returned. - static PyObject *release(py_ref_tmpl &&ref) noexcept + static PyObject *release(py_ref_t &&ref) noexcept { // If the contained reference should not be null, assert that it isn't. PYDYND_ASSERT_IF(not_null, ref.o != nullptr); @@ -404,18 +404,18 @@ class py_ref_tmpl { // Convenience aliases for the templated smart pointer classes. -using py_ref = py_ref_tmpl; +using py_ref = py_ref_t; -using py_ref_with_null = py_ref_tmpl; +using py_ref_with_null = py_ref_t; -using py_borref = py_ref_tmpl; +using py_borref = py_ref_t; -using py_borref_with_null = py_ref_tmpl; +using py_borref_with_null = py_ref_t; template -PyObject *release(py_ref_tmpl &&ref) +PyObject *release(py_ref_t &&ref) { - return py_ref_tmpl::release(std::forward>(ref)); + return py_ref_t::release(std::forward>(ref)); } /* Capture a new reference if it is not null. @@ -435,10 +435,10 @@ inline py_ref capture_if_not_null(PyObject *o) * only checks for via an assert statement in debug builds. */ template -inline py_ref_tmpl nullcheck(py_ref_tmpl &&obj) noexcept(not_null) +inline py_ref_t nullcheck(py_ref_t &&obj) noexcept(not_null) { // Route this through the assignment operator since the semantics are the same. - py_ref_tmpl out = obj; + py_ref_t out = obj; return out; } @@ -447,13 +447,13 @@ inline py_ref_tmpl nullcheck(py_ref_tmpl &&o * This should be used when the pointer is already known to not be null. */ template -inline py_ref_tmpl disallow_null(py_ref_tmpl &&obj) noexcept +inline py_ref_t disallow_null(py_ref_t &&obj) noexcept { // Assert that the wrapped pointer is actually not null. assert(obj.get() != nullptr); // Assert that the wrapped reference is valid if it is not null. PYDYND_ASSERT_IF(obj.get() != nullptr, Py_REFCNT(obj.get()) > 0); - return py_ref_tmpl(obj.release(), owns_ref); + return py_ref_t(obj.release(), owns_ref); } // RAII class to acquire GIL. From 495faf4a78ab59df169b7f591286be9acb737a3d Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 17:34:09 -0600 Subject: [PATCH 13/18] Add a method to get an owned reference from a given reference. --- dynd/include/utility_functions.hpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index a463b9cf..bfd471f4 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -384,7 +384,21 @@ class py_ref_t { } } - py_ref_t borrow() noexcept { return py_ref(o, false); } + // Return a wrapped borrowed reference to the wrapped reference. + py_ref_t borrow() noexcept + { + // Assert that the wrapped reference is actually valid if it isn't null. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); + return py_ref_t(o, false); + } + + // Return a wrapped owned reference referring to the currently wrapped reference. + py_ref_t borrow() noexcept + { + // Assert that the wrapped reference is actually valid if it isn't null. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); + return py_ref_t(o, false); + } // Return a reference to the encapsulated PyObject as a raw pointer. // Set the encapsulated pointer to NULL. From a1aa4059471bd91007e6b889e0cd7a60b7fde7f9 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 17:38:50 -0600 Subject: [PATCH 14/18] Add method to py_ref_t to create a new owned reference from a given reference. --- dynd/include/utility_functions.hpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index bfd471f4..474e59c4 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -387,14 +387,18 @@ class py_ref_t { // Return a wrapped borrowed reference to the wrapped reference. py_ref_t borrow() noexcept { + // Assert that the wrapped pointer is not null if it shouldn't be. + PYDYND_ASSERT_IF(not_null, o != nullptr); // Assert that the wrapped reference is actually valid if it isn't null. PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); return py_ref_t(o, false); } // Return a wrapped owned reference referring to the currently wrapped reference. - py_ref_t borrow() noexcept + py_ref_t new_ownref() noexcept { + // Assert that the wrapped pointer is not null if it shouldn't be. + PYDYND_ASSERT_IF(not_null, o != nullptr); // Assert that the wrapped reference is actually valid if it isn't null. PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); return py_ref_t(o, false); From 7962d97683627e0c8045eac9ddba840a67497632 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 17:42:44 -0600 Subject: [PATCH 15/18] Add method to py_ref_t to copy the current reference. --- dynd/include/utility_functions.hpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 474e59c4..543400b2 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -404,6 +404,16 @@ class py_ref_t { return py_ref_t(o, false); } + // Copy the current reference. + py_ref_t copy() noexcept + { + // Assert that the wrapped pointer is not null if it shouldn't be. + PYDYND_ASSERT_IF(not_null, o != nullptr); + // Assert that the wrapped reference is actually valid if it isn't null. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); + return py_ref_t(o, false); + } + // Return a reference to the encapsulated PyObject as a raw pointer. // Set the encapsulated pointer to NULL. // If this is a type that owns its reference, an owned reference is returned. From fc522f45c9d617b0b43f184c5c9adb0ac8797c4a Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Thu, 9 Jun 2016 18:07:32 -0600 Subject: [PATCH 16/18] Fix typo in disallow_null where release was not correctly used. --- dynd/include/utility_functions.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 543400b2..bc3a079b 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -481,7 +481,7 @@ inline py_ref_t disallow_null(py_ref_t &&obj assert(obj.get() != nullptr); // Assert that the wrapped reference is valid if it is not null. PYDYND_ASSERT_IF(obj.get() != nullptr, Py_REFCNT(obj.get()) > 0); - return py_ref_t(obj.release(), owns_ref); + return py_ref_t(release(std::forward(obj)), owns_ref); } // RAII class to acquire GIL. From 9e8bf428640643b7137a8ef1e2e748ad8af9a961 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Wed, 22 Jun 2016 17:16:52 -0600 Subject: [PATCH 17/18] Close a reference leak in smart pointers for managing PyObject pointers. Fix some erroneous noexcept statements. Also add some more helpful assert statements. --- dynd/include/utility_functions.hpp | 36 +++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index bc3a079b..229f9129 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -258,6 +258,7 @@ class py_ref_t { */ explicit py_ref_t(PyObject *obj, bool consume_ref) noexcept { + PYDYND_ASSERT_IF(not_null, obj != nullptr); o = obj; if (consume_ref) { decref_if_owned(o); @@ -278,13 +279,6 @@ class py_ref_t { decref_if_owned(o); } - // For assignment operators, only allow assignment - // in cases where implicit conversions are also allowed. - // This forces explicit handling of cases that could - // need to have an exception raised. - // For that reason, these are all marked as noexcept. - // Assignment never comsumes a reference. - /* If: * This type allows null or the input type does not, * Then: @@ -314,7 +308,7 @@ class py_ref_t { */ template * = nullptr> - py_ref_t &operator=(const py_ref_t &other) noexcept + py_ref_t &operator=(const py_ref_t &other) { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -332,6 +326,8 @@ class py_ref_t { } // Same as previous two, except these assign from rvalues rather than lvalues. + // Only allow move assignment if the input type owns its reference. + // Otherwise, the copy assignment operator is sufficient. /* If: * this type allows null, @@ -341,9 +337,8 @@ class py_ref_t { * Then: * Assignment from the other py_ref_t type to this type may not raise an exception. */ - template * = nullptr> - py_ref_t &operator=(py_ref_t &&other) noexcept + template * = nullptr> + py_ref_t &operator=(py_ref_t &&other) noexcept { // If the input reference should not be null, assert that that is the case. PYDYND_ASSERT_IF(other_not_null, other.o != nullptr); @@ -354,7 +349,12 @@ class py_ref_t { decref_if_owned(o); o = other.o; other.o = nullptr; - incref_if_owned(o); + // If type being assigned to does not hold its own reference, + // Decref the input reference. + decref_if_owned(o); + // Assert that, after possibly decrefing the input reference, + // that the stored reference is still valid. + PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0); return *this; } @@ -364,9 +364,8 @@ class py_ref_t { * Then: * Assignment from the other py_ref_t type to this type may raise an exception. */ - template * = nullptr> - py_ref_t &operator=(py_ref_t &&other) noexcept + template * = nullptr> + py_ref_t &operator=(py_ref_t &&other) { if (other.o != nullptr) { // Assert that the input reference is valid. @@ -376,7 +375,12 @@ class py_ref_t { decref_if_owned(o); o = other.o; other.o = nullptr; - incref_if_owned(o); + // If the type being assigned to does not own its own reference, + // decref the wrapped pointer. + decref_if_owned(o); + // Assert that the wrapped reference is still valid after + // any needed decrefs. + assert(Py_REFCNT(o) > 0); return *this; } else { From 4d9ec89a7c00766d77ac8d00f7a0b3fdf3851e09 Mon Sep 17 00:00:00 2001 From: Ian Henriksen Date: Wed, 3 Aug 2016 17:48:19 -0500 Subject: [PATCH 18/18] Add more commenting to the new py_ref classes. --- dynd/include/utility_functions.hpp | 49 +++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/dynd/include/utility_functions.hpp b/dynd/include/utility_functions.hpp index 229f9129..fb582e92 100644 --- a/dynd/include/utility_functions.hpp +++ b/dynd/include/utility_functions.hpp @@ -123,6 +123,26 @@ inline void decref_if_owned(PyObject *obj) noexcept PYDYND_ASSERT_IF(obj != nullptr, Py_REFCNT(obj) > 0); } +/* A template that provides smart pointers with semantics that match + * all of the following: + * - an owned reference to a Python object that cannot be null + * - an owned reference to a Python object that may possibly be null + * - a borrowed reference to a Python object that cannot be null + * - a borrowed reference to a Python object that may possibly be null + * The resulting smart pointers can be used to create wrappers + * for existing functions that use the CPython API that effectively + * encode null-checking and reference counting semantics for a given + * function into its type signature. + * Implicit conversions are used to allow functions wrapped in this + * way to accept smart pointers of varied semantics. The implicit + * conversions take care of things like raising exceptions when a + * null valued pointer is converted to one that should not be null. + * These smart pointers are also designed to allow controlling + * the lifetime of a given smart pointer in a safe way. + * In debug mode, assertions are used wherever possible to + * make absolutely certain that a given reference is valid whenever + * it is moved from, copied, accessed, etc. + */ template class py_ref_t { PyObject *o; @@ -132,6 +152,7 @@ class py_ref_t { // This allows the smart pointer class to be moved from. // Whether or not the class is allowed to be null only changes // when zerochecks/exceptions may occur and when assertions are used. + // The null state always represents an "empty" smart pointer. py_ref_t() noexcept : o(nullptr){}; // First define an accessor to get the PyObject pointer from @@ -145,8 +166,8 @@ class py_ref_t { } // All copy and move constructors are designated implicit. - // Unless they convert between a null type to a not_null type, - // they are also declared noexcept. + // They are also declared noexcept unless they convert + // from a possibly null type to a not_null type. /* If: * This type allows null or the input type does not, @@ -165,7 +186,6 @@ class py_ref_t { incref_if_owned(o); } - // Should this one be declared explicit since it can throw? /* If: * this type doesn't allow null, * and the input type does, @@ -192,6 +212,11 @@ class py_ref_t { // from types that own their references. // Only define them for those cases. + // Allow moving from an owned reference to a borrowed reference. + // Though this would do the same thing as a copy constructor in that case, + // it does make it possible to assert that the pointer is still valid + // after the decref to turn the owned reference into a borrowed reference. + /* If: * the input type owns its reference, * and we are not moving from a type that allows null values to one that does not, @@ -279,6 +304,10 @@ class py_ref_t { decref_if_owned(o); } + // Assignment between wrapped references with different semantics is allowed. + // It will raise an exception if a null valued pointer is assigned to a + // pointer type that does not allow nulls. + /* If: * This type allows null or the input type does not, * Then: @@ -329,6 +358,13 @@ class py_ref_t { // Only allow move assignment if the input type owns its reference. // Otherwise, the copy assignment operator is sufficient. + // Move assignment from an owned reference to a borrowed reference is allowed + // primarily so that an assertion can be used to check that the reference is + // valid after decref is used to convert it to a borrowed reference. + // This is important because it can raise a meaningful assertion error whenever + // anyone tries to put a newly created reference into a pointer type + // designed to hold a borrowed reference. + /* If: * this type allows null, * Or: @@ -444,6 +480,8 @@ using py_borref = py_ref_t; using py_borref_with_null = py_ref_t; +// Convert a given smart pointer back to a raw pointer, +// releasing its reference if it owns one. template PyObject *release(py_ref_t &&ref) { @@ -452,6 +490,8 @@ PyObject *release(py_ref_t &&ref) /* Capture a new reference if it is not null. * Throw an exception if it is. + * This is meant to be used to forward exceptions raised in Python API + * functions that are signaled via null return values. */ inline py_ref capture_if_not_null(PyObject *o) { @@ -476,7 +516,8 @@ inline py_ref_t nullcheck(py_ref_t &&obj) no /* Convert to a non-null reference. * No nullcheck is used, except for an assertion in debug builds. - * This should be used when the pointer is already known to not be null. + * This should be used when the pointer is already known to not be null + * and the type does not yet reflect that. */ template inline py_ref_t disallow_null(py_ref_t &&obj) noexcept