For c2py of stl types take argument by value and move elements#35
For c2py of stl types take argument by value and move elements#35
Conversation
Wentzell
force-pushed
the
PR_C2PY_BY_VALUE
branch
from
01cd04c to
b6b3393
Compare
|
Passes all triqs + cthyb tests |
|
This fixes TRIQS/triqs#755 |
parcollet
left a comment
parcollet
left a comment
There was a problem hiding this comment.
Started the review.
Several fine points to discuss, not completely obvious to me
| } | ||
| static PyObject *c2py(std::optional<T> op) { | ||
| if (!bool(op)) Py_RETURN_NONE; | ||
| return conv::c2py(std::move(*op)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There should be no problem with the syntax.
std::move(*op)We invoke
T& operator*() &;and then move from the T&.
| 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))); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Even if its safe this will most definitely trigger false-positives for static analyzers / warnings that warn about use after move!?
|
@parcollet There is in fact one more aspect about this PR that needs to be discussed. We will no longer be able to e.g. convert a member |
Wentzell
force-pushed
the
PR_C2PY_BY_VALUE
branch
from
24259b0 to
09c266a
Compare
Wentzell
force-pushed
the
PR_C2PY_BY_VALUE
branch
from
09c266a to
74fc95a
Compare
|
Most points here have been addressed. The Pull Request #37 is in fact an extension of this PR, so discussion should be continued there. I will close this as a duplicate. |
2 participants
No description provided.