overloaded 'operator+' must be a unary or binary operator error

Question

Following the advice given in this answer, I have overloaded the + operator in my simple Point class as follows (the += overload works fine).

Point operator+ (Point p1, const Point& p2)
{
    return std::move(p1 += p2);
}

But I get an error saying

overloaded 'operator+' must be a unary or binary operator (has 3 parameters)

What is wrong?

Cavill

The advice is wrong. Never move return values, this is a unnecessary code bloat and makes them even slower. – Extravascular

@Extravascular That was something else I was hoping to spark a little debate about – Cavill

@Extravascular without a move that would have copied. He would have to split his code into two lines to achieve the same effect (it is not allowed to do NRVO from a function parameter, but the rvalue trasformation is still done). – Fellmonger

@JohannesSchaub-litb Yes, I need to p1+=p2; return p1; now, but I would also need to do the same with move – Cavill

Answer 1

It sounds like you have declared your operator as a member function. A member function takes an implicit first parameter, meaning your operator now takes three parameters. You can fix this by making it a non-member function.

In any case, it is preferable to declare it as a non-member, to ensure symmetry between the LHS and the RHS of the operation.

As for std::move, it is in the <utility> header. Although I can't see the reason to use it here.

Undershrub

Well, the operators can be members but in this case the implicitly passed object counts as one argument already. – Mcnabb

@DietmarKühl You're right, of course, they can be members, but better have symmetry between LHS and RHS. – Undershrub

Indeed it's generally considered best to have them as free functions, but you don't need to. – Logic

@LightnessRacesinOrbit Agreed. I edited the answer to reflect this. – Undershrub

That overload of std::move is not relevant; the one pertinent here is in <utility>. – Rainproof

Answer 2

You want to do either:

// Member function, performs (*this + right)
Point operator+ (Point & right)

or

// Free function, performs (left + right)
Point operator+ (const Point &left, const Point& right)

Overripe

What's the need for friend here? – Logic

@LightnessRacesinOrbit a hack so you can declare the function inside the class declaration :-) Nothing to do with friendship. – Undershrub

I must add, I don't think friend is needed here at all, it is just confusing. Plus the signature change is significant here. – Undershrub

@Undershrub declaring the function within a class implies access to private members. That has already been done. Using friend removes the implicit this from that situation. – Overripe

But you don't need access to private members. Therefore, you shouldn't have any. Basic principles of encapsulation... – Logic

I think it was a programming error to declare it as a member. In any case, += is usually public. – Undershrub

@Undershrub I'm sure that you're right about that. I think my answer directly answered the question with the least changes, but perhaps a note would have helped along the lines of "You should ask a new question about why this function should be declared outside of the class" – Overripe

Answer 3

You made the operator a member function, meaning it actually has three parameters when you include the implicit first this parameter.

Either:

• Use *this rather than p1 and get rid of that first parameter, or
• Make the operator overload a free function (instead of a member) — this is preferred.

Logic

@ildjarn: Fracking oops. Ta :) – Logic

Answer 4

The member function implicitly takes this as a parameter,

So take operator+ as a member function, should only have one explicit parameter:

void Point::operator+(const Point& rhs)
{
    ...
}

And if take operator+ as a non-member function, sholud have two parameters:

Point operator+(Point& lhs, const Point& rhs)
{
    ...
}

And for std::move, it casts rvalue reference to rvalue, and move lhs into return value, only reasonable to use when lhs passed as rvalue reference:

Point operator+(Point&& lhs, const Point& rhs)
{
    lhs += rhs;
    return std::move(lhs);
}

Or you may consider use std::forward, if pass lhs as lvalue reference and rvalue reference both exits:

template<
    typename T,
    typename = typename std::enable_if<
        std::is_same<Point, typename std::decay<T>::type>::value
        >::type>
Point
operator+(T&& lhs,  const Point& rhs)
{
    lhs += rhs;
    return std::forward<T>(lhs);  // move rvalue into return value, copy lvalue
}

Refer to Effective Modern C++ by Scott Meyers item 25: Use std::move on rvalue references, std::forward on universal references.

Bettencourt