Fix unary operators

[zwimer]

This fixes: #225

It also addresses #226 by renaming plus_plus to pre_increment or post_increment depending on which is called. It unfortunately does not fix the issue that the post increment operator requires an integer argument. I'm not sure how to fix that. If this is not ok, I can undo this fix and only let this PR address #225

[zwimer]

Likewise, the above applies for minus_minus

[zwimer]

@lyskov If you have a suggestion as to how to make the post_increment and post_decrement not take in a dummy-integer argument, please let me know and I'll do it!

To be clear, I change binder so that:

1. ++x: operator++() no longer maps to plus_plus but instead maps to pre_increment
2. x++: operator++(int) no longer maps to plus_plus but instead maps to post_increment

The issue is that dummy int C++ post-increment operator requires is taken literally by binder so post_increment needs an argument, even though it's just a dummy that is ignored.

[lyskov]

@zwimer thank you for reporting this!

Re proposed solution: i thought about this and have rather mixed feeling about it: technically it possible to provide value for this dummy parameter (and for operator to use it) (see https://en.cppreference.com/w/cpp/language/operator_incdec). So why it might be more "pythonic" to avoid user to specify it, it in the same time disallow us to fully use the C++ code. Of course we could probably all agree that using this dummy int as parameter in C++ code is probably a bad idea in the first place... but as usual in such cases when we disallow such usage it will break someone workflow. (btw this point is nicely illustrate in https://xkcd.com/1172/)

[zwimer]

@lyskov I think I was unclear. Right now, if you bind both ++a and a++, binder will generate something akin to:

cl.def("plus_plus", (struct T & (T::*)()) &T::operator++, "C++: T::operator++() --> struct T &", pybind11::return_value_policy::automatic);
cl.def("plus_plus", (struct T & (T::*)(int)) &T::operator++, "C++: T::operator++(int) --> struct T &", pybind11::return_value_policy::automatic, pybind11::arg(""));

This PR changes that to

cl.def("pre_incerement", (struct T & (T::*)()) &T::operator++, "C++: T::operator++() --> struct T &", pybind11::return_value_policy::automatic);
cl.def("post_increment", (struct T & (T::*)(int)) &T::operator++, "C++: T::operator++(int) --> struct T &", pybind11::return_value_policy::automatic, pybind11::arg(""));

I think it would be beneficial to somehow instead do:

cl.def("pre_incerement", (struct T & (T::*)()) &T::operator++, "C++: T::operator++() --> struct T &", pybind11::return_value_policy::automatic);
cl.def("post_increment", (struct T & (T::*)()) []()->T(*)(int) { return &T::operator++; }(), "C++: T::operator++(int) --> struct T &", pybind11::return_value_policy::automatic);

That is to say. Currently in python we have: a.plus_plus(); a.plus_plus(0) for pre/post increment. This PR would give a.pre_increment(); a.post_increment(0). I think it would be better if we could edit this PR so that python would only need a.pre_increment(); a.post_increment().

My problem is that I do not know how to go from what this PR does to the better solution of not requiring a dummy argument in the first place. While I do think the current PR is better than what is on master, if it is an easy fix to make this other improvement, I think that is best. Thoughts?

[zwimer]

Oh, no, I think I see your point. Hmm, is it possible / a good idea to let that be a CLI flag? Or bind both?

[lyskov]

Oh, no, I think I see your point. Hmm, is it possible / a good idea to let that be a CLI flag? Or bind both?

-- let me think about this. Right now my take is that both naming (plus_plus vs pre/post_increment) is rather arbitrary choice so it is hard to argue while one is better than another. And there is also issue of backward compatibility with such renaming so with all things being about equal using old names is preferable.

[zwimer]

@lyskov For now I'll split this into 2 PRs. One that just fixes the bugs, and one that does this renaming.

[zwimer]

@lyskov The ++ and -- stuff has been moved to #238 It is was removed from this PR. This PR is not exclusively the bug fixes of the unary ops.

[lyskov]

Could you please update comments, particularly elaborate on how selection of correct name from the vector will be correlated to number of arguments C++ operator have? Thanks,

[lyskov]

Have you considered approach:

const auto n = F.getNumParams();
return n < vec.size() ? vec[n] : vec.back();

• that way operators with non-standard type signature (more then two arguments, operator with default arguments) will also be bound and could be used in Python by calling corresponding magic-method.

[zwimer]

That will work as well, before if you wanted a catch all you would just put it as the sole argument but I can update it to this. I don't think it would make a difference since I don't recall C++ allowing additional operator parameters besides the ones I coded, but I suppose this is safer in case I missed anything.

[lyskov]

LGTM! - Thank you for putting this together @zwimer !