Skip to content

For c2py of stl types take argument by value and move elements #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

For c2py of stl types take argument by value and move elements #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61b727a
For c2py of stl types take argument by value and move elements
Wentzell Jun 5, 2020
74fc95a
Fix in vector.hpp converter
Wentzell Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions c++/cpp2py/converters/map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ namespace cpp2py {

template <typename K, typename V> struct py_converter<std::map<K, V>> {

static PyObject *c2py(std::map<K, V> const &m) {
static PyObject *c2py(std::map<K, V> m) {
PyObject *d = PyDict_New();
for (auto &x : m) {
pyref k = py_converter<K>::c2py(x.first);
pyref k = py_converter<std::decay_t<K>>::c2py(std::move(x.first));
// if the K is a list, we transform into a tuple
if (PyList_Check(k)) k = PyList_AsTuple(k);
pyref v = py_converter<V>::c2py(x.second);
pyref v = py_converter<std::decay_t<V>>::c2py(std::move(x.second));
if (k.is_null() or v.is_null() or (PyDict_SetItem(d, k, v) == -1)) {
Py_DECREF(d);
return NULL;
Expand Down
10 changes: 5 additions & 5 deletions c++/cpp2py/converters/optional.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ namespace cpp2py {

template <typename T> struct py_converter<std::optional<T>> {

using conv = py_converter<T>;
using conv = py_converter<std::decay_t<T>>;

static PyObject *c2py(std::optional<T> &op) {
if (!bool(op)) Py_RETURN_NONE;
return conv::c2py(*op);
}
static PyObject *c2py(std::optional<T> op) {
if (!bool(op)) Py_RETURN_NONE;
return conv::c2py(std::move(*op));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't rather
std::move(op).value() or *(std::move(op)) [ the second is somewhat less clear]
you have to move the op itself I think.. Not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no problem with the syntax.

std::move(*op)

We invoke

T& operator*() &;

and then move from the T&.

}

static bool is_convertible(PyObject *ob, bool raise_exception) {
return ((ob == Py_None) or conv::is_convertible(ob, raise_exception));
Expand Down
6 changes: 3 additions & 3 deletions c++/cpp2py/converters/pair.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ namespace cpp2py {

template <typename T1, typename T2> struct py_converter<std::pair<T1, T2>> {

static PyObject *c2py(std::pair<T1, T2> const &p) {
pyref x1 = py_converter<T1>::c2py(std::get<0>(p));
pyref x2 = py_converter<T2>::c2py(std::get<1>(p));
static PyObject *c2py(std::pair<T1, T2> p) {
pyref x1 = py_converter<std::decay_t<T1>>::c2py(std::move(std::get<0>(p)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question:
I would have said :
std::get<0>(std::move(p))
move p first, then using (4) in https://en.cppreference.com/w/cpp/utility/pair/get

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with std::get<1>(std::move(p)) we then use a p which has already been moved from? Isn't this something we want to avoid at all cost? Is this well-defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if its safe this will most definitely trigger false-positives for static analyzers / warnings that warn about use after move!?

pyref x2 = py_converter<std::decay_t<T2>>::c2py(std::move(std::get<1>(p)));
if (x1.is_null() or x2.is_null()) return NULL;
return PyTuple_Pack(2, (PyObject *)x1, (PyObject *)x2);
}
Expand Down
4 changes: 2 additions & 2 deletions c++/cpp2py/converters/set.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ namespace cpp2py {

template <typename K> struct py_converter<std::set<K>> {

static PyObject *c2py(std::set<K> const &s) {
static PyObject *c2py(std::set<K> s) {
PyObject *set = PySet_New(NULL);
for (auto &x : s) {
pyref y = py_converter<K>::c2py(x);
pyref y = py_converter<std::decay_t<K>>::c2py(std::move(x));
if (y.is_null() or (PySet_Add(set, y) == -1)) {
Py_DECREF(set);
return NULL;
Expand Down
6 changes: 3 additions & 3 deletions c++/cpp2py/converters/std_array.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ namespace cpp2py {
template <typename T, size_t R> struct py_converter<std::array<T, R>> {
// --------------------------------------

static PyObject *c2py(std::array<T, R> const &v) {
static PyObject *c2py(std::array<T, R> v) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait...
std::array is STACK object, you can not really move it always copy.
What shall we do here ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this will in fact lead to an additional unnecessary stack copy.
Can we really avoid providing all three overloads in these cases?

c2py(std::array<T, R> &v)
c2py(std::array<T, R> const &v)
c2py(std::array<T, R> &&v)

PyObject *list = PyList_New(0);
for (auto const &x : v) {
pyref y = py_converter<T>::c2py(x);
for (auto &x : v) {
pyref y = py_converter<std::decay_t<T>>::c2py(std::move(x));
if (y.is_null() or (PyList_Append(list, y) == -1)) {
Py_DECREF(list);
return NULL;
Expand Down
10 changes: 5 additions & 5 deletions c++/cpp2py/converters/tuple.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,28 @@ namespace cpp2py {
using tuple_t = std::tuple<Types...>;

// c2py implementation
template <std::size_t... Is> static PyObject *c2py_impl(tuple_t const &t, std::index_sequence<Is...>) {
auto objs = std::array<pyref, sizeof...(Is)>{pyref(py_converter<Types>::c2py(std::get<Is>(t)))...};
template <std::size_t... Is> static PyObject *c2py_impl(tuple_t t, std::index_sequence<Is...>) {
auto objs = std::array<pyref, sizeof...(Is)>{pyref(py_converter<std::decay_t<Types>>::c2py(std::move(std::get<Is>(t))))...};
bool one_is_null = std::accumulate(std::begin(objs), std::end(objs), false, [](bool x, PyObject *a) { return x or (a == NULL); });
if (one_is_null) return NULL;
return PyTuple_Pack(sizeof...(Types), (PyObject *)(objs[Is])...);
}

// is_convertible implementation
template <int N, typename T, typename... Tail> static bool is_convertible_impl(PyObject *seq, bool raise_exception) {
return py_converter<T>::is_convertible(PySequence_Fast_GET_ITEM(seq, N), raise_exception)
return py_converter<std::decay_t<T>>::is_convertible(PySequence_Fast_GET_ITEM(seq, N), raise_exception)
&& is_convertible_impl<N + 1, Tail...>(seq, raise_exception);
}
template <int> static bool is_convertible_impl(PyObject *seq, bool raise_exception) { return true; }

template <size_t... Is> static auto py2c_impl(std::index_sequence<Is...>, PyObject *seq) {
return std::make_tuple(py_converter<Types>::py2c(PySequence_Fast_GET_ITEM(seq, Is))...);
return std::make_tuple(py_converter<std::decay_t<Types>>::py2c(PySequence_Fast_GET_ITEM(seq, Is))...);
}

public:
// -----------------------------------------

static PyObject *c2py(tuple_t const &t) { return c2py_impl(t, std::make_index_sequence<sizeof...(Types)>()); }
static PyObject *c2py(tuple_t t) { return c2py_impl(std::move(t), std::make_index_sequence<sizeof...(Types)>()); }

// -----------------------------------------

Expand Down
10 changes: 3 additions & 7 deletions c++/cpp2py/converters/variant.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,14 @@ namespace cpp2py {
}

struct _visitor {
template <typename U> PyObject *operator()(U const &x) { return py_converter<U>::c2py(x); }
template <typename U> PyObject *operator()(U x) { return py_converter<std::decay_t<U>>::c2py(std::move(x)); }
};

public:
static PyObject *c2py(std::variant<T...> const &v) {
//auto l = [](auto const &x) -> PyObject * { return py_converter<decltype(x)>::c2py(x); };
return visit(_visitor{}, v);
//return visit(_visitor{}, v);
}
static PyObject *c2py(std::variant<T...> v) { return visit(_visitor{}, std::move(v)); }

static bool is_convertible(PyObject *ob, bool raise_exception) {
if ((... or py_converter<T>::is_convertible(ob, false))) return true;
if ((... or py_converter<std::decay_t<T>>::is_convertible(ob, false))) return true;
if (raise_exception) { PyErr_SetString(PyExc_TypeError, "Cannot convert to std::variant"); }
return false;
}
Expand Down
21 changes: 10 additions & 11 deletions c++/cpp2py/converters/vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,22 @@
namespace cpp2py {

template <typename T> static void delete_pycapsule(PyObject *capsule) {
auto *ptr = static_cast<std::unique_ptr<T[]> *>(PyCapsule_GetPointer(capsule, "guard"));
auto *ptr = static_cast<std::vector<T> *>(PyCapsule_GetPointer(capsule, "guard"));
delete ptr;
}

// Convert vector to numpy_proxy, WARNING: Deep Copy
template <typename T> numpy_proxy make_numpy_proxy_from_vector(std::vector<T> const &v) {
template <typename T> numpy_proxy make_numpy_proxy_from_vector(std::vector<T> v) {

auto *data_ptr = new std::unique_ptr<T[]>{new T[v.size()]};
std::copy(begin(v), end(v), data_ptr->get());
auto capsule = PyCapsule_New(data_ptr, "guard", &delete_pycapsule<T>);
auto *vec_heap = new std::vector<T>{std::move(v)};
auto capsule = PyCapsule_New(vec_heap, "guard", &delete_pycapsule<T>);

return {1, // rank
npy_type<std::remove_const_t<T>>,
(void *)data_ptr->get(),
(void *)vec_heap->data(),
std::is_const_v<T>,
v_t{static_cast<long>(v.size())}, // extents
v_t{sizeof(T)}, // strides
v_t{static_cast<long>(vec_heap->size())}, // extents
v_t{sizeof(T)}, // strides
capsule};
}

Expand All @@ -46,14 +45,14 @@ namespace cpp2py {

template <typename T> struct py_converter<std::vector<T>> {

static PyObject *c2py(std::vector<T> const &v) {
static PyObject *c2py(std::vector<T> v) {

if constexpr (has_npy_type<T>) {
return make_numpy_proxy_from_vector(v).to_python();
return make_numpy_proxy_from_vector(std::move(v)).to_python();
} else { // Convert to Python List
PyObject *list = PyList_New(0);
for (auto const &x : v) {
pyref y = py_converter<T>::c2py(x);
pyref y = py_converter<std::decay_t<T>>::c2py(std::move(x));
if (y.is_null() or (PyList_Append(list, y) == -1)) {
Py_DECREF(list);
return NULL;
Expand Down