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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 24, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd9bae4
Add operator= to shared_ptr for assignments to base classes
fuglede May 21, 2021
8d9ff80
Add test for shared_ptr assignment to base classes
fuglede May 21, 2021
be5e625
Change return value of operator= for shared_ptr
fuglede May 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Cython/Includes/libcpp/memory.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ cdef extern from "<memory>" namespace "std" nogil:
shared_ptr(shared_ptr[T]&, T*)
shared_ptr(unique_ptr[T]&)
#shared_ptr(weak_ptr[T]&) # Not Supported
T& operator=[Y](const shared_ptr[Y]& ptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@fuglede fuglede May 22, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


# Modifiers
void reset()
Expand Down
9 changes: 9 additions & 0 deletions tests/run/cpp_smart_ptr.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ cdef cppclass C(B):

cdef shared_ptr[A] holding_subclass = shared_ptr[A](new C())


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


def test_dynamic_pointer_cast():
"""
>>> test_dynamic_pointer_cast()
Expand Down