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

Spurious temporaries break non-copyable types #3253

Open
pitrou opened this issue Nov 28, 2019 · 3 comments
Open

Spurious temporaries break non-copyable types #3253

pitrou opened this issue Nov 28, 2019 · 3 comments

Comments

@pitrou
Copy link
Contributor

@pitrou pitrou commented Nov 28, 2019

If I have a C++ API such as the following:

cdef extern from 'arrow/util/compression.h' namespace 'arrow' nogil:
    cdef cppclass CCodec" arrow::util::Codec":
        @staticmethod
        CResult[unique_ptr[CCodec]] Create(CompressionType codec)

cdef extern from "arrow/result.h" namespace "arrow" nogil:
    cdef cppclass CResult "arrow::Result"[T]:
        pass

cdef extern from "arrow/python/common.h" namespace "arrow::py" nogil:
    T GetResultValue[T](CResult[T]) except *

Then the following Cython code:

    cdef:
        CompressionType compression_type = ...
        unique_ptr[CCodec] c_codec
    c_codec = GetResultValue(CCodec.Create(compression_type))

creates this C++ code:

  __pyx_t_6 = arrow::py::GetResultValue<std::unique_ptr< arrow::util::Codec> >( arrow::util::Codec::Create(__pyx_v_compression_type)); if (unlikely(PyErr_Occurred())) __PYX_ERR(6, 1203, __pyx_L1_error)
  __pyx_v_codec = __pyx_t_6;

which fails compiling because it tries to copy a unique_ptr.
Cython should instead move the temporary:

  __pyx_t_6 = arrow::py::GetResultValue<std::unique_ptr< arrow::util::Codec> >( arrow::util::Codec::Create(__pyx_v_compression_type)); if (unlikely(PyErr_Occurred())) __PYX_ERR(6, 1203, __pyx_L1_error)
  __pyx_v_codec = std::move(__pyx_t_6);

(I can add the move() call explicitly in the Cython source, but it's a bit surprising, and also it's also inconvenient because of #2169)

@da-woods

This comment has been minimized.

Copy link
Contributor

@da-woods da-woods commented Nov 28, 2019

I believe this is a bug in your code: except * means "can raise a Python exception with any return value". Since you wouldn't expect __pyx_v_codec to be assigned if the function raises a Python exception then the code generated is correct.

You want except + which means "can raise a C++ exception" - https://cython.readthedocs.io/en/latest/src/userguide/wrapping_CPlusPlus.html#exceptions

@pitrou

This comment has been minimized.

Copy link
Contributor Author

@pitrou pitrou commented Nov 28, 2019

You want except + which means "can raise a C++ exception"

No, I do mean a Python exception.

Since you wouldn't expect __pyx_v_codec to be assigned if the function raises a Python exception then the code generated is correct.

I don't see how that relates. Even if it's not assigned to, then you still can std::move(it).

Moreover, in the case where the object is copyable, the copying may be expensive, while moving is cheap, so moving is better in every case.

@da-woods

This comment has been minimized.

Copy link
Contributor

@da-woods da-woods commented Nov 28, 2019

OK sorry I misunderstood. The existence of the temporary is right, because it's matching the following Python behaviour:

def f():
    raise RuntimeError()

def g():
    try:
        a = f()
    except:
        # "a" shouldn't be assigned here

If the temporary didn't exist then a would be assigned directly exception or not and the behaviour would be incorrect. I think we both agree on this bit, but just making sure.

I'm not 100% sure what C++ standard Cython targets as a baseline. I'm pretty sure it still targets C89, so on that basis I'd assume it aims for the earliest possible C++ standard, and that std::move might not be acceptable for generated code. But I don't know this for certain so I'll stop commenting now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.