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.

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

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

Closed
josephsnyder opened this issue Sep 29, 2020 · 7 comments
Closed

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

josephsnyder opened this issue Sep 29, 2020 · 7 comments

Comments

@josephsnyder
Copy link
Owner

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
Copy link
Collaborator

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

@EricCousineau-TRI
Copy link
Collaborator

Here's basic operator overloading + refs in namespace:
EricCousineau-TRI/repro@446faac

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Oct 1, 2020

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
Copy link
Collaborator

@josephsnyder
Copy link
Owner Author

@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
Copy link
Collaborator

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: EricCousineau-TRI/repro@be6641559
(2) We can still resolve it using ADL: EricCousineau-TRI/repro@8b5f3e9
(3) But cannot refer to it via global operator+: EricCousineau-TRI/repro@290c6b9
(4) Nor via scoped nested::operator+: EricCousineau-TRI/repro@5eb7abf
(5) Nor via scoped nested::C::operator+: EricCousineau-TRI/repro@d6c1539
(6) But using ADL, we can write a strictly C-compatible lambda: EricCousineau-TRI/repro@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
Copy link
Collaborator

EricCousineau-TRI commented Jan 28, 2021

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

No branches or pull requests

2 participants