[ENH] Add operator= to shared_ptr for assignments to base classes

[fuglede]

As discussed in this StackOverflow answer, having the operator= around enables us to assign shared_ptr[Derived] to shared_ptr[Base].

Now admittedly, as the question will suggest, I'm new to C++ Cython and in particular I would be unable to see what pitfalls this might have, but it does work as intended in my particular use case.

[scoder]

Thanks. Could you add a test in tests/run/cpp_smart_ptr.pyx ?
You can run it with python runtests.py -vv --backend=cpp cpp_smart_ptr.

[fuglede]

I've added a test which fails without the one-line addition. However, the test below, which does the opposite -- assigns the base pointer to the pointer to the derived -- actually goes through as well even though that looks like something that should fail.

def test_assignment_to_derived_class():
    """
    >>> test_assignment_to_derived_class()
    """
    cdef shared_ptr[A] base = shared_ptr[A](new A())
    cdef shared_ptr[C] derived = base

Edit: Well, okay, the incorrect test is green, but it doesn't actually compile with cythonize, cf. below (but the correct one compiles fine). I don't know enough Cython to say why that is.

C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29910\include\memory(1797): error C2440: '<function-style-cast>': cannot convert from 'const std::shared_ptr<__pyx_t_4test_A>' to 'std::shared_ptr<__pyx_t_4test_C>'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29910\include\memory(1797): note: No constructor could take the source type, or constructor overload resolution was ambiguous
test.cpp(1900): note: see reference to function template instantiation 'std::shared_ptr<__pyx_t_4test_C> &std::shared_ptr<__pyx_t_4test_C>::operator =<__pyx_t_4test_A>(const std::shared_ptr<__pyx_t_4test_A> &) noexcept' being compiled

[da-woods]

def test_assignment_to_derived_class():
    """
    >>> test_assignment_to_derived_class()
    """
    cdef shared_ptr[A] base = shared_ptr[A](new A())
    cdef shared_ptr[C] derived = base

When I try this test it passes through Cython but fails with the C++ compiler (which I think is what you find too). I think that's expected and OK - Cython doesn't/can't have enough information to work out what pointer types are compatible, so all it can do is assume the user knows.

[da-woods]

Shouldn't this return a shared_ptr[T]& ?

And arguably there's a matching constructor too? (although a little harder to use from Cython since most things end up default-constructed)

[fuglede]

Thanks for the review!

Shouldn't this return a shared_ptr[T]&?

Hm, again, I'm a Cython newb so I'm not sure -- certainly what you're saying looks more logical and closer to what's in the reference. But I've tried both and cythonize will generate the same cpp in both cases in the test_assignment_to_base_class example.

And arguably there's a matching constructor too?

Do you mean shared_ptr[Y](shared_ptr[Y]&)? As the SO poster remarks, that doesn't quite work.

        shared_ptr[Y](shared_ptr[Y]&)
                   ^
memory.pxd:64:20: unknown type in template argument

...

        shared_ptr[Y](shared_ptr[Y]&)
                                 ^
memory.pxd:64:34: unknown type in template argument
...
        shared_ptr[Y](shared_ptr[Y]&)
                    ^
memory.pxd:64:21: Missing name in declaration.

[da-woods]

Hm, again, I'm a Cython newb so I'm not sure -- certainly what you're saying looks more logical and closer to what's in the reference. But I've tried both and they generate the same cpp in the case of the test_assignment_to_base_class example.

It should only matter in for multiple assignments (or similar). So for test_assignment_to_base_class it makes no difference, but for second_copy = first_copy = original it might make a difference (I think... I haven't looked into the exact code that Cython generates here).

Do you mean shared_ptr[Y](shared_ptr[Y]&)? As the SO poster remarks, that doesn't quite work.

I'd think it should be shared_ptr(shared_ptr[Y]&) (note the first square brackets gone). If that doesn't work then it's fine - don't spend more time on it though. The point the SO poster is making is that Cython generally generates c++ code like:

// start of function
shared_ptr<A> a;
// function body
a = something;

and so operator= is generally much more useful.

[fuglede]

for second_copy = first_copy = original it might make a difference

I just tried

    cdef shared_ptr[C] derived = shared_ptr[C](new C())
    cdef shared_ptr[A] base1
    cdef shared_ptr[A] base2
    base1 = base2 = derived

Here, the generated code will once again be the same for both return types. In any case, I've changed it to shared_ptr[T]&.

If that doesn't work then it's fine

Did try that one as well; doesn't quite work:

        shared_ptr(shared_ptr[Y]&)
                              ^
memory.pxd:64:31: unknown type in template argument

don't spend more time on it though

I don't mind; this is quite helpful for learning what's going on!

[scoder]

Thanks. Looks ok to me.

It seems a rather broad change – the way I understand it, it allows assigning anything to anything, right? But users should rarely run into issues here, and the C++ compiler will catch them.

[fuglede]

it allows assigning anything to anything, right?

Yeah, cf. the example in this comment above, it's lenient, but indeed, that one is caught during compilation.