-
Notifications
You must be signed in to change notification settings - Fork 10
cast: Ensure that pointers do not use rvalue / move overload #28
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
cast: Ensure that pointers do not use rvalue / move overload #28
Conversation
|
Thanks! |
|
Submitted PR; updated this overview to indicate that it's a prerequisite. |
e4cb38c to
a99f997
Compare
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m-chaturvedi)
a discussion (no related file):
In the downstream Drake PR, I'm still confused as to why we need py_reference when returning T* from a cpp_function.
Might you be able to add a unittest here in pybind11 that tests returning / casting T* for a class with holder type unique_ptr, and a type that is non-copyable and non-moveable?
(Let me know if you'd like help on this part)
|
a discussion (no related file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
(And, uh, you may have already mentioned the reason beforehand... It's still fuzzy in my mind when reviewing the code / convos...) |
9729855 to
de49686
Compare
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @m-chaturvedi)
a discussion (no related file):
Made some suggested changes: m-chaturvedi#1
Are you OK with these?
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m-chaturvedi)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
(And, uh, you may have already mentioned the reason beforehand... It's still fuzzy in my mind when reviewing the code / convos...)
OK I tried out an non-moveable and non-copyable type, same code paths. Thanks for your patience!
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending remaining nits in meta-PR mentioned below
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m-chaturvedi)
|
I apologize for not figuring this out before, but it looks like Pybind11 seems to provide some classes for testing. We could forgo some of our code. I tried to implement your comments in this: |
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those look good! Can you push them here?
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m-chaturvedi)
de49686 to
b893bd3
Compare
|
a discussion (no related file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Added the comments, the issue link and changed the message in the new commit. |
m-chaturvedi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry for the delay!
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
Towards Issue 9398
Requires:
upstream/master(35045ee) from previous merge-base (f2bd883) #29 (CI fixes)This change is