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

std::move missing from libcpp #2169

Closed
pitrou opened this issue Mar 21, 2018 · 4 comments · Fixed by #3358
Closed

std::move missing from libcpp #2169

pitrou opened this issue Mar 21, 2018 · 4 comments · Fixed by #3358

Comments

@pitrou
Copy link
Contributor

pitrou commented Mar 21, 2018

Since it seems tricky to find the right declaration spelling for std::move (if at all possible?), it would be nice to have it predeclared in libcpp.

@ozars
Copy link
Contributor

ozars commented Jul 23, 2019

I attempted declaring std::move with no luck. I got it working through a wrapper, but I'm not sure if it covers all cases or causes subtle bugs. Here is the idea:

This is the definition of std::move:

template< class T >
typename std::remove_reference<T>::type&& move( T&& t ) noexcept;

This is test.pyx file I will use for demonstration:

# distutils: language = c++

cdef extern from *:
    """
    #include <iostream>

    #define PRINT() std::cout << __PRETTY_FUNCTION__ << std::endl

    struct Test {
        Test() { PRINT(); }
        ~Test() { PRINT(); }
        Test(const Test&) { PRINT(); }
        Test(Test&&) { PRINT(); }
        Test& operator=(const Test&) { PRINT(); return *this; }
        Test& operator=(Test&&) { PRINT(); return *this; }
    };

    void f(const Test&) { PRINT(); }
    void f(Test&&) { PRINT(); }
    """

    cdef cppclass Test:
        pass

    cdef void f(Test)

from move cimport move

cdef Test t1, t2

print("# t1 = t2")
t1 = t2

print("# t1 = move(t2)")
t1 = move(t2)

print("# f(t1)")
f(t1)

print("# f(move(t1))")
f(move(t1))

print("# f(move(move(t1)))")
f(move(move(t1)))

print("# f(move(move(move(t1))))")
f(move(move(move(t1))))

First attempt at move.pxd:

# distutils: language = c++

cdef extern from "<utility>" namespace "std" nogil:
    cdef T&& move[T](T&&)

Compiling test.pyx returns bunch of errors, but the first one was:

/home/omer/tmp/cythonize/test.cpp: In function ‘int __pyx_pymod_exec_test(PyObject*)’:                                                                                                                             
/home/omer/tmp/cythonize/test.cpp:1541:54: error: no matching function for call to ‘move<Test>(Test&)’                                                                                                             
   __pyx_v_4test_t1 = std::move<Test>(__pyx_v_4test_t2);                                              

So, the issue is that cython explicitly provides template argument Test, however t1 cannot bind to the function parameter of type Test&&. For this to work, the function call should have been made without explicit template argument or with correct explicit template argument. I think both of these requires some changes in the way cython translates the code.

Another way would be having a wrapper which accepts above template parameter and calls std::move correctly. An initial wrapper:

# distutils: language = c++

cdef extern from * namespace "polyfill":
    """
    #include <type_traits>
    #include <utility>

    namespace polyfill {

    template <typename T>
    inline typename std::remove_reference<T>::type&& move(T& t) {
        return std::move(t);
    }

    template <typename T>
    inline typename std::remove_reference<T>::type&& move(T&& t) {
        return std::move(t);
    }

    }  // namespace pf
    """
    cdef T&& move[T](T&)
    cdef T&& move[T](T&&)

This causes error for three level nested move:

/home/omer/tmp/cythonize/test.cpp: In function ‘int __pyx_pymod_exec_test(PyObject*)’:
/home/omer/tmp/cythonize/test.cpp:1631:27: error: cannot declare reference to ‘struct Test&&’, which is not a typedef or a template type argument
   f(polyfill::move<Test &&&&>(polyfill::move<Test &&>(polyfill::move<Test>(__pyx_v_4test_t1))));
                           ^~

So the return value being declared as xvalue is the culprit. From what I understood it doesn't matter if a function's parameter/return type is xvalue, prvalue or lvalue from cython's point of view, so we can simplify above code as:

# distutils: language = c++

cdef extern from * namespace "polyfill":
    """
    #include <type_traits>
    #include <utility>

    namespace polyfill {

    template <typename T>
    inline typename std::remove_reference<T>::type&& move(T& t) {
        return std::move(t);
    }

    template <typename T>
    inline typename std::remove_reference<T>::type&& move(T&& t) {
        return std::move(t);
    }

    }  // namespace pf
    """
    cdef T move[T](T)

It compiles and works correctly:

 ~/tmp/cythonize  python3 -c "import test"
Test::Test()
Test::Test()
# t1 = t2
Test& Test::operator=(const Test&)
# t1 = move(t2)
Test& Test::operator=(Test&&)
# f(t1)
void f(const Test&)
# f(move(t1))
void f(Test&&)
# f(move(move(t1)))
void f(Test&&)
# f(move(move(move(t1))))
void f(Test&&)
Test::~Test()
Test::~Test()

I can submit a PR for embedding above wrapper snippet into libcpp.utility if above approach looks alright.

@ozars
Copy link
Contributor

ozars commented Jul 24, 2019

I packaged it as cymove for now.

@robertwb
Copy link
Contributor

Thanks for looking into this. We'd welcome a PR to put the solution with the template wrapper into Cython itself.

@ozars
Copy link
Contributor

ozars commented Feb 17, 2020

@robertwb

I submitted a PR over the weekend. It's ready to be merged if there are no changes needed. I'd appreciate if you could review it.

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

Successfully merging a pull request may close this issue.

4 participants