What's is wrong with overloaded operator?

Question

I'm learn C++ (overloading operators if be precise). I try overload operator+ by this way:

Complex4d Complex4d::operator+(const Complex4d &rvalue)
{
    return Complex4d(a() + rvalue.a(), b());
}

Where rvalue.a() and a(), rvalue.b() and b() it's object of the Complex2d. In Complex2d class I overload operator+ too, by this way:

Complex2d Complex2d::operator +(Complex2d &rvalue)
{
    return Complex2d(a() + rvalue.a(), b() + rvalue.b());
}

If I write this:

Complex4d Complex4d::operator+(const Complex4d &rvalue)
{
    Complex2d test = rvalue.a();
    return Complex4d(a() + test, b());
}

All it's OK. What do i do wrong?

`operator+` should generally be a free function that just does `return lhs += rhs;`. — chris, Jul 09 '13 at 19:31
Do you mean, that i need realize operator+ as out the class function, but not as member of class? — Сергей Волков, Jul 09 '13 at 19:34
Yes, it has several advantages that way, including encapsulation and being more lenient with the left side. — chris, Jul 09 '13 at 19:39
You don't *have* to, but you should if you want the operator to be completely symmetric to LHS and RHS. — juanchopanza, Jul 09 '13 at 19:39
@chris well `std::move(lhs+=rhs)` with `lhs` taken by-value -- `+=` returns a `Foo&`, and you want to move this into the return value. — Yakk - Adam Nevraumont, Jul 09 '13 at 19:44
@Yakk, Wouldn't the compiler do that anyway if RVO is out of the question? — chris, Jul 09 '13 at 19:48
@chris: no, `lhs` is an lvalue, so it wouldn't be moved by default. — Mooing Duck, Jul 09 '13 at 19:49
@MooingDuck, Interesting, and moving it inhibits RVO AFAIK, so is RVO not going to happen here anyway? — chris, Jul 09 '13 at 19:50
@chris: I don't see why moving would affect RVO. I think RVO should happen here. (Keep in mind `std::move` doesn't actually move things, it merely marks them as movable) — Mooing Duck, Jul 09 '13 at 19:52
@СергейВолков, Your `Complex2d` one should take a const reference. You can't bind temporaries to non-const references. — chris, Jul 09 '13 at 19:52
@chris: Good eye! I was baffled by this question until you spotted that! Make that an answer. — Mooing Duck, Jul 09 '13 at 19:53
@MooingDuck, I swear there was something about explicitly moving into a return value not allowing RVO to occur. — chris, Jul 09 '13 at 19:53
What do you get from compiler?? What error? could you put it here? — Leandro Lima, Jul 09 '13 at 20:10
@MooingDuck there is something about moving inhibiting RVO in the selected answer to [this question](http://stackoverflow.com/questions/4986673/c11-rvalues-and-move-semantics-confusion) and the comments. — juanchopanza, Jul 09 '13 at 20:11
@Yakk, You should see the link as well. It kind of forces a choice then. — chris, Jul 09 '13 at 20:13
Well, without know what is the error it is kind difficult to understand. I think that is probably a error with const argument. — Leandro Lima, Jul 09 '13 at 20:15
@juanchopanza: returning a local can do NRVO. moving a local prevents NRVO. "Normal" RVO kicks in when you return a temporary, and moving doesn't affect that. In this code, he's returning a temporary. — Mooing Duck, Jul 09 '13 at 20:16
@MooingDuck, That makes everything fit so much better into place, thanks. We must reach a definite conclusion on the matter! — chris, Jul 09 '13 at 20:17
@MooingDuck to me, that seems to contradict what is stated in the answer. "The std::move on tmp is unnecessary and can actually be a performance pessimization as it will inhibit return value optimization." — juanchopanza, Jul 09 '13 at 20:19
@juanchopanza, I'd take that to mean NRVO with just a pedantically wrong piece of terminology used in the wording. I really don't know for sure (yet) whether there's a difference, but I definitely want there to be now. — chris, Jul 09 '13 at 20:21
@juanchopanza: `return Complex4d(...);` would use RVO. `return move(Complex4d(...))` doesn't change anything, and still uses RVO. `return lhs;` would use NRVO. `return move(lhs);` _prevents NRVO from kicking in_, and so copy/moves. `return lhs+=rhs;` can use neither RVO nor NRVO, and so copy/moves. `return move(lhs+=rhs)` can't use RVO nor NRVO and so copy/moves. — Mooing Duck, Jul 09 '13 at 20:25
@MooingDuck, Apparently not due to the reference. I'm starting to think a separate question would be a better area for this discussion. I don't think we really have anything addressing that. Want me to make one? — chris, Jul 09 '13 at 20:28
@chris: not according to what reference? Howard Hinnat? Other than the fact he said "RVO" instead of "NRVO", I'm agreeing with him 100%. `move` prevents NRVO. Making a question might be a good idea. — Mooing Duck, Jul 09 '13 at 20:29
The real trick should be `lhs+=rhs; return lhs;`, which doesn't block NRVO and implicitly `move`s. (`lhs+=rhs` returns an lvalue reference to `lhs`, which can neither be NRVO'd or `move`d implicitly). Still, `std::move( lhs += rhs )` or `std::move(lhs) == rhs` somehow seems more moving. — Yakk - Adam Nevraumont, Jul 09 '13 at 20:30
@MooingDuck, The reference returned by `operator+=` :p That's the argument Yakk is making, anyway. — chris, Jul 09 '13 at 20:30

score 4 · Accepted Answer · edited Jul 10 '13 at 07:20

4

The problem is that you're trying to bind a temporary to a non-const reference, which is not allowed and makes no sense:

Complex2d Complex2d::operator +(Complex2d &rvalue)
                                ^^^^^^^^^^^
return Complex4d(a() + rvalue.a(), b());
                       ^^^^^^^^^^

To fix it, make it take a const reference, to which temporaries can be bound. Const-correctness also applies. If you're not modifying it (you shouldn't be), make it const.

Complex2d Complex2d::operator +(const Complex2d &rvalue)
                                ^^^^^

The other argument (*this) isn't modified either:

Complex2d Complex2d::operator +(const Complex2d &rvalue) const
                                ^^^^^                    ^^^^^

As well, I suggest making them free functions and reusing other code:

Complex2d operator+(const Complex2d &lhs, const Complex2d &rhs) {
    auto ret = lhs;
    ret += rhs;
    return ret; //see comments for implementation reasoning
}

This allows the left side to act the same as the right and improves encapsulation by having one less unnecessary function with access to the private members of the class.

edited Jul 10 '13 at 07:20

MSalters

159,923
8
140
320

answered Jul 09 '13 at 19:58

chris

55,166
13
130
185

Calling `+=` on a `const` reference won't work very well :-) I would pass by value, and just `return lhs += rhs`. – juanchopanza Jul 09 '13 at 20:03
@juanchopanza, Oops, did I do that? One sec. – chris Jul 09 '13 at 20:04
Better! So is the `move` in case there is no RVO? – juanchopanza Jul 09 '13 at 20:05
@juanchopanza, See the comments on the question about that. Either I remember correctly and now there cannot be RVO, or, more likely, my memory sucks and now it will have a possibility of either. – chris Jul 09 '13 at 20:05
[this post](http://stackoverflow.com/questions/4986673/c11-rvalues-and-move-semantics-confusion) seems to argue against moving in this case. I suspect Hinnant knows his stuff too :) – juanchopanza Jul 09 '13 at 20:08
@juanchopanza, That's what I thought, except it won't be moved automatically, so I guess you're left with a choice of RVO or copy-construction. I'd definitely rely on RVO getting it. In any case, I made RVO the default and put a note as a comment. – chris Jul 09 '13 at 20:12
@juanchopanza: I _think_ Hinnant is actually talking about NRVO when he says of RVO in that post. [So does this guy](http://stackoverflow.com/questions/4986673/c11-rvalues-and-move-semantics-confusion#comment21299985_4986802) – Mooing Duck Jul 09 '13 at 20:17
@MooingDuck Right, so we have a named value, `lhs`, which could have been NRVOed without the `std::move`, and isn't anymore. Surely that is a bad thing. – juanchopanza Jul 09 '13 at 20:25
The reason you need that `move` is that `lhs += rhs` returns a `Complex2d &`, not a temporary unless you carefully wrote a pair of rvalue and lvalue `+=` operators. Now, you could write `Complex2d operator+=( Complex2d const& rhs )&&`, but I'm not sure if that is better than `Complex2d&& operator+=( Complex2d const& rhs )&&` in terms of avoiding object-`move`s, as we (probably) cannot elide `*this` into the return value of `+=`, can we? If we instead did `lhs+=rhs; return lhs;` we would avoid this `move`ing controversy. – Yakk - Adam Nevraumont Jul 09 '13 at 20:27
@Yakk, That probably would be better if you need the `move` call. – chris Jul 09 '13 at 20:29
@Yakk right, I might have taken the "a reference is an alias for an object" a bit too far. In my confused mind `return lhs += rhs;` was the same as `lhs += rhs; return lhs;`. – juanchopanza Jul 09 '13 at 20:32
@juanchopanza: when the compiler compiles `return lhs+=rhs;`, it simply _references_ `Complex2d::operator+=`, and although it knows it returns a reference, but _doesn't know which reference is being returned_, so can't shortcut the copy. In `return lhs;`, it clearly knows that whever `operator+=` returns, _this_ function should return `lhs`, so it can apply NRVO on `lhs`. – Mooing Duck Jul 09 '13 at 20:34
There, I updated my answer with that version. It certainly looks better than the move, and doesn't require you to include an additional header, but you'll definitely have to put a comment in, as that's certainly an obvious simplification. – chris Jul 09 '13 at 20:39
I am starting to wonder about the wisdom of passing one argument by value vs. copying it into a temporary in the body of the function. See [here](http://ideone.com/n0IzlU). – juanchopanza Jul 09 '13 at 21:07
@juanchopanza, Honestly, my mind has already blown up today :p – chris Jul 09 '13 at 21:16
Right, passing the argument to be returned by value inhibits RVO. There's a nice explanation about it [here](http://definedbehavior.blogspot.ch/2011/08/value-semantics-copy-elision.html). – juanchopanza Jul 10 '13 at 05:13
@juanchopanza, Geez, so many conflicting things. So much for "Want Speed? Pass By Value" in this case :p – chris Jul 10 '13 at 05:17

What's is wrong with overloaded operator?

1 Answers1