Skip to content
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.

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

Move arguments instead of copy #1612

Closed
hmenke opened this issue Feb 23, 2017 · 1 comment · Fixed by #3362
Closed

Move arguments instead of copy #1612

hmenke opened this issue Feb 23, 2017 · 1 comment · Fixed by #3362
Labels

Comments

@hmenke
Copy link

hmenke commented Feb 23, 2017

Consider the following class in C++. Note that the class is not copy constructable as it contains a managed pointer. The constructor of the function may throw an exception.

#pragma once
#include <iostream>
#include <memory>

class TestClass
{
public:
  std::unique_ptr<int> i;

  TestClass() : i(new int) {};

  TestClass(int i_) : i(new int)
  {
    if ( i_ == 0 )
      throw 0;
    *i = i_;
  }
};

inline void cpp_function(TestClass t)
{
  std::cout << *(t.i) << "\n";
}

This exception should be propagated to Cython, which should be possible via except +.

cdef extern from "test.hpp":
    cppclass TestClass:
        TestClass(int) except +

    void cpp_function(TestClass t)
        
def interface(i):
     cpp_function(TestClass(i))

Unfortunately, the generated code cannot be compiled. Let’s take a look why. The relevant part is this (shortened) bit:

static PyObject *__pyx_pf_4test_interface(CYTHON_UNUSED PyObject *__pyx_self, PyObject *__pyx_v_i) {
  /* ... some more stuff ... */
  TestClass __pyx_t_2;
  __Pyx_RefNannySetupContext("interface", 0);

  /* "test.pyx":8
 * 
 * def interface(i):
 *      cpp_function(TestClass(i))             # <<<<<<<<<<<<<<
 */
  __pyx_t_1 = __Pyx_PyInt_As_int(__pyx_v_i); if (unlikely((__pyx_t_1 == (int)-1) && PyErr_Occurred())) __PYX_ERR(0, 8, __pyx_L1_error)
  try {
    __pyx_t_2 = TestClass(__pyx_t_1);
  } catch(...) {
    __Pyx_CppExn2PyErr();
    __PYX_ERR(0, 8, __pyx_L1_error)
  }
  cpp_function(__pyx_t_2);
  /* ... some more stuff ... */
}

First of all we notice the line

TestClass __pyx_t_2;

which implies that the object is default constructable. If not, the code will not compile. A meaningless default constructor can in most cases be achieved but puts some additional bookkeeping work on the C++ programmer’s shoulders. This is also an issue but can be circumvented.

More crucial is the line

cpp_function(__pyx_t_2);

where the object is attempted to be copied. This is not possible as the object is not copyable. The code does not compile, because error: use of deleted function ‘TestClass::TestClass(const TestClass&)’.

With C++11 this situation can be handled though, simply using

cpp_function(std::move(__pyx_t_2));

In case that the object can be moved this is done, otherwise it is copied nevertheless.

Please add std::move to function calls.


Addendum:

It would be best to use a pointer for the object to be created in combination with the move. This way it does neither need to be default constructable nor copyable.

static PyObject *__pyx_pf_4test_interface(CYTHON_UNUSED PyObject *__pyx_self, PyObject *__pyx_v_i) {
  /* ... some more stuff ... */
  std::unique_ptr<TestClass> __pyx_t_2;
  __Pyx_RefNannySetupContext("interface", 0);

  /* "test.pyx":8
 * 
 * def interface(i):
 *      cpp_function(TestClass(i))             # <<<<<<<<<<<<<<
 */
  __pyx_t_1 = __Pyx_PyInt_As_int(__pyx_v_i); if (unlikely((__pyx_t_1 == (int)-1) && PyErr_Occurred())) __PYX_ERR(0, 8, __pyx_L1_error)
  try {
    __pyx_t_2.reset(new TestClass(__pyx_t_1));
  } catch(...) {
    __Pyx_CppExn2PyErr();
    __PYX_ERR(0, 8, __pyx_L1_error)
  }
  cpp_function(std::move(*__pyx_t_2));
  /* ... some more stuff ... */
}
@scoder
Copy link
Contributor

scoder commented Apr 20, 2020

Not sure if this is completely solved by #3362, but seems worth a second look once that is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants