josephsnyder / drake-external-examples

Examples of how to use Drake in your own project.
https://drake.mit.edu/
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Wrapping "friend" operators without py::self shortcuts #3

Closed josephsnyder closed 3 years ago

josephsnyder commented 4 years ago

When trying to wrap the polynomial class, we receive a variety of errors with the direct generation of the pybind11 code.

Wrapping drake::Polynomial<double> const drake::operator+(double const & scalar, drake::Polynomial<double> const & p) [free operator] which points to this function: https://github.com/RobotLocomotion/drake/blob/master/common/polynomial.h#L341

returns

/home/softhat/Work/TRI/drake-external-examples/drake_cmake_bindings/build/pydrake/polynomial/Polynomial_py.cpp:59:144: error: ‘operator+’ is not a member of ‘drake’
     .def("__add__", static_cast<::drake::Polynomial<double> const (*)( ::drake::Polynomial<double> const &,double const & )>(&::drake::operator+), py::arg("p"), py::arg("scalar"))
                                                                                                                                                ^
/home/softhat/Work/TRI/drake-external-examples/drake_cmake_bindings/build/pydrake/polynomial/Polynomial_py.cpp:59:144: note: suggested alternatives:
In file included from /home/softhat/Work/TRI/drake-pybind/include/pybind11/stl.h:20:0,
                 from /home/softhat/Work/TRI/drake-external-examples/drake_cmake_bindings/build/pydrake/polynomial/Polynomial_py.cpp:3:
/usr/include/c++/7/valarray:1172:1: note:   ‘std::operator+’
 _DEFINE_BINARY_OPERATOR(+, __plus)

Additionally, "operator<<" returns a slightly different error:

/home/softhat/Work/TRI/drake-external-examples/drake_cmake_bindings/build/pydrake/polynomial/Polynomial_py.cpp:68:134: error: invalid static_cast from type ‘<unresolved overloaded function type>’ to type ‘std::ostream& (*)(std::ostream&, const drake::Polynomial<double>&) {aka std::basic_ostream<char>& (*)(std::basic_ostream<char>&, const drake::Polynomial<double>&)}’
     .def("__lshift__", static_cast<::std::ostream & (*)( ::std::ostream &,::drake::Polynomial<double> const & )>(&::drake::operator<<), py::arg("os"), py::arg("poly"))
EricCousineau-TRI commented 4 years ago

Hmm... Can you show a MWE (minimum working example) of this in a standalone C++ program?

EricCousineau-TRI commented 4 years ago

Here's basic operator overloading + refs in namespace: https://github.com/EricCousineau-TRI/repro/commit/446faace64d19af4ecb948d8af88d8ff2a8f83a3

EricCousineau-TRI commented 4 years ago

And here's it breaking (for g++ and clang++) if operator decl+def moves into a friend thing: EricCousineau-TRI/repro@fbf112f

So at least it's nothing special to Drake?

EricCousineau-TRI commented 4 years ago

See also: https://gitlab.kitware.com/autopybind11/autopybind11/-/issues/130

josephsnyder commented 4 years ago

@EricCousineau-TRI, it does now! Correct, I do not believe it's drake specific. But PyBind11, or maybe AutoPyBind11, aren't happy with friend operators.

I know about issue 130. I created it to have something to point to for our timing system when working on this particular issue.

As for the MWE, I've pushed a commit to my fork of autopybind here: https://gitlab.kitware.com/joe-snyder/autopybind11/-/commit/0117429a99d299312d4aa35a8971ad17677a36a5 This shows the operator+ problem when running. I've also included the autopybind output file named simple_py.cpp.

I'm working on getting the operator<< one to error in a minimal way.

EricCousineau-TRI commented 4 years ago

Copying email thread:


From @bradking:

According to the C++ standard paragraph 7.3.1.2/3, friend injection only makes the name visible in the namespace for argument-dependent lookup, but not for qualified or unqualified name lookup. The latter two are only available after an explicit declaration of the name appears literally in the namespace.

I don't think argument-dependent lookup can be used to take the address of a function, so there is no way to do what you want without a real declaration in the namespace.


Follow-up on my part:

Thanks! Corroborating Brad's comment above with some add'l checks:

(1) Explicitly adding a declaration (not definition) works: https://github.com/EricCousineau-TRI/repro/commit/be6641559 (2) We can still resolve it using ADL: https://github.com/EricCousineau-TRI/repro/commit/8b5f3e9 (3) But cannot refer to it via global operator+: https://github.com/EricCousineau-TRI/repro/commit/290c6b9 (4) Nor via scoped nested::operator+: https://github.com/EricCousineau-TRI/repro/commit/5eb7abf (5) Nor via scoped nested::C::operator+: https://github.com/EricCousineau-TRI/repro/commit/d6c1539 (6) But using ADL, we can write a strictly C-compatible lambda: https://github.com/EricCousineau-TRI/repro/commit/96d1e9d

So perhaps simplest thing is to rely on ADL + lambdas; perhaps even simpler would be to use py::self, iff the types are default-constructible?

(I'll try to find a reference on +[](...), I had used it before for some NumPy stuff...)

P.S. This is part of the motivation for listing the various function-binding options here: https://drake.mit.edu/doxygen_cxx/group__python__bindings.html#PydrakeOverloads


Follow-up on my part:

Er, this is what I found on +[]; I guess not a language construct in itself, but a little jig to force the lambda in the right direction: https://stackoverflow.com/a/18889029/7829525


\cc @bradking @billhoffman @jamiesnape

EricCousineau-TRI commented 3 years ago

Closing per GitLab issue: https://gitlab.kitware.com/autopybind11/autopybind11/-/issues/130