Skip to content

Update from upstream master (test_multiple_inheritance.cpp). #53

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

Conversation

rwgk
Copy link

@rwgk rwgk commented Mar 7, 2021

Importing pybind#2890.


This change is Reviewable

@rwgk
Copy link
Author

rwgk commented Mar 7, 2021

Hi @EricCousineau-TRI, the GitHub UI here won't let me assign a reviewer. How does it work here?

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Mar 8, 2021

Thanks Ralf!
Given that pybind#2890 is just some shuffling, is it OK if we hold off on this PR until there's a more substantial upstream update? (it requires some manual downstream testing on my part that I'd like to hold off on for now)

@EricCousineau-TRI EricCousineau-TRI self-requested a review March 8, 2021 18:54
@rwgk
Copy link
Author

rwgk commented Mar 16, 2021

Hi @EricCousineau-TRI, I looked into the drake-vs-smart_holder merge conflicts for pybind11.h. Currently there are 3 conflicts in pybind11.h, 2 are easy to resolve, but the one below looks like a show stopper, because you changed the interface of init_instance. I have SFINAE alternatives for the smart_holder code, which means your interface change will impact the smart_holder code one way or another (maybe it can be kept local to pybind11.h, I'm not sure). I see you also changed the interfaces of other functions from const void *holder_ptr to detail::holder_erased holder_ptr. Some of those are called from the smart_holder code, so those will be tricky to handle, too.

I will keep this PR open just in case you find it useful to have the resolved merge conflict for test_multiple_inheritance.cpp ready to copy over into your next drake-merge-master PR. Feel free to close this PR anytime.

<<<<<<< drake                                                                    
    static void init_instance(detail::instance *inst, detail::holder_erased holder_ptr) {
        using namespace detail;                                                 
=======                                                                         
    template <                                                                  
        typename T = type,                                                      
        detail::enable_if_t<!detail::type_uses_smart_holder_type_caster<T>::value, int> = 0>
    static void init_instance(detail::instance *inst, const void *holder_ptr) { 
>>>>>>> smart_holder                                                            

@EricCousineau-TRI
Copy link
Collaborator

Howdy Ralf! Sorry for delay here; this change will be incorporated in #57 (w/ merge conflicts), so I'll go ahead and close this PR.

Regarding the other merge conflicts, it's still on my plate to replace our smart ptr hacks w/ your better design, but just haven't gotten to it yet - thanks!

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

Successfully merging this pull request may close these issues.

2 participants