3

I was doing this as a personal exercise and wanted to make sure I got this right and understand it correctly. I have a coordinate class with members row and column. I wanted to overload the + and += operator. Here is my code:

Coordinate& Coordinate :: operator+= (const Coordinate& rhs){
    this->m_Row += rhs.m_Row;
    this->m_Column += rhs.m_Column;

    return *this;
}

Coordinate& operator+ (const Coordinate& lhs, const Coordinate& rhs) {
    return Coordinate(lhs) += rhs;
}

where 

friend Coordinate& operator + (const Coordinate& lhs, const Coordinate& rhs);

is a friend function defined in Coordinate class.

Are there any pitfalls to this code ?

Here is my understanding of how these work:

operator +=

Add rhs m_Row and m_Column to this members. Return a reference to the object pointed by this pointer and thereby avoid creating another object due to copy constructor.

operator +

Create a local object of lhs (since lhs is a constant and we don't want to modify its contents) using the copy constructor (lets call it localObj). Invoke the += member operator on localObj which performs the addition. Return a reference to this localObj so that we don't create another object due to copy constructor, otherwise.

Now, the very last statement concerns me, since I am returning a reference to a local object. As soon as the function (operator +) goes out of scope, localObj will be destroyed and returned reference will point to an object which has been destroyed. Am I correct in understanding this ?

If so, how should I fix it ??

EDIT: After all the answers and what I learnt: here is what my Coordinate class looks like now: http://rextester.com/MJJI7394

brainydexter
  • 19,826
  • 28
  • 77
  • 115

2 Answers2

6

You are right to be worried, you are returning a reference to a temporary here:

Coordinate& operator+ (const Coordinate& lhs, const Coordinate& rhs) {
    return Coordinate(lhs) += rhs;
}

You need to return a Coordinate by value, for example like this:

Coordinate operator+ (Coordinate lhs, const Coordinate& rhs) {
    return lhs += rhs;
}

In the example above, we make a copy of the first parameter instead of taking a reference and then copying in the body of the function. Then we return the result of += on that, by value.

With this setup, there is no need to declare operator+ as a friend.

See this SO link for more information, and thanks to @Blastfurnace for pointing it out.

Community
  • 1
  • 1
juanchopanza
  • 223,364
  • 34
  • 402
  • 480
  • why is it a temporary? in `a=b+c;` wont his operator return a reference to b? – Gir Aug 11 '12 at 17:54
  • 1
    @Gir it is a temporary because it is a `Coordinate` object in the body of the function, which is returned by reference. The object dies when we exit the function scope, so the reference on the caller side is left dangling. – juanchopanza Aug 11 '12 at 17:57
  • why is it in the body of the function? it recieves lhs, which is b, by reference. Then the += operator adds rhs, which is c, and returns the reference to lhs. i dont see any variables allocated inside the function that would die with it. ah nm. now noticed the copy ctor – Gir Aug 11 '12 at 18:00
  • Thanks for clarifying that. 2 questions now: 1. When we return by value, we end up calling the copy constructor and this means creating extra objects ? 2. In this `Coordinate operator+ (Coordinate lhs, const Coordinate& rhs) `, why did you not use `Coordinate& lhs` ? – brainydexter Aug 11 '12 at 18:04
  • 1
    @Gir This `Coordinate(lhs) += rhs` only lives in the scope of the function, yet a reference to it is returned. That is bad. – juanchopanza Aug 11 '12 at 18:04
  • @Gir: In the expression `a=b+c;` nobody expects `b` to be changed as a side effect. You have to return a new object with the result. juanchopanza's second code snippet is the simplest correct way to do that. – Blastfurnace Aug 11 '12 at 18:05
  • 2
    @brainydexter Yes, and no. The compiler is allowed to do [copy elision](http://en.wikipedia.org/wiki/Copy_elision), particularly *return value optimization*. Why not pass `lhs` by reference? Because we cannot modify it, and we will probably end up making a copy anyway, so passing by value lets the compiler perform more copy elisions than if we pass by reference. – juanchopanza Aug 11 '12 at 18:07
  • Aha! Gotchya, My last query being then, when would I want to return by reference ? (*Scratching my head*) – brainydexter Aug 11 '12 at 18:09
  • Also, would you recommend me to go about implementing these two operators like this or in some other different way ? – brainydexter Aug 11 '12 at 18:10
  • 1
    @brainydexter: There are good guidelines in this [operator overloading question](http://stackoverflow.com/questions/4421706/operator-overloading). Defining `operator+` in terms of `operator+=` is a recommended practice. – Blastfurnace Aug 11 '12 at 18:13
  • @brainydexter you return by reference when it makes sense semantically and you actually have something you can return. `operator+=` is a classic example, because it modifies the object itself, so you can return it (you could return void, but returning the reference lets you chain expressions together). `operator+` should not modify `lhs` or `rhs`. So you need to return a new instance, hence you return by value. – juanchopanza Aug 11 '12 at 18:15
  • An excellent answer. But just to mention it, when writing the code this way, there's no need for `operator=` to be a friend. – James Kanze Aug 11 '12 at 18:27
0

Personally, I would define operator+=() in terms of operator+() and operator=():

Coordinate operator+(const Coordinate& lhs, const Coordinate& rhs) {
  return Coordinate(lhs.getRow() + rhs.getRow(), lhs.getCol() + rhs.getCol();
}

const Coordinate& operator=(Coordinate& lhs, const Coordinate& rhs) {
  lhs.setRow(rhs.getRow());
  lhs.setCol(rhs.setCol());

  return lhs;
}

const Coordinate& operator+=(Coordinate& lhs, const Coordinate&rhs) {
  return lhs = lhs + rhs;
}

I'm using setters and getters here. Alternatively, you can use friend and/or member functions. Note that all functions that return references are returning the parameters that are sent in so that there are no issues with references to local or temporary objects.

Code-Apprentice
  • 81,660
  • 23
  • 145
  • 268
  • 1
    Personally, why would you do that? Now your `operator+=` makes an unnecessary copy followed by an unnecessary assignment. – Benjamin Lindley Aug 11 '12 at 18:14
  • This code won't compile. You are trying to change `lhs` which is a __const reference__. – Blastfurnace Aug 11 '12 at 18:15
  • I would disagree with this (although I have seen it done in many places). This is a [good link](http://stackoverflow.com/questions/4421706/operator-overloading). – juanchopanza Aug 11 '12 at 18:17
  • @Blastfurnace Oops! I fixed it now. – Code-Apprentice Aug 11 '12 at 18:17
  • @BenjaminLindley Perhaps it isn't the most efficient, but it is cleaner to read, IMO. – Code-Apprentice Aug 11 '12 at 18:18
  • @juanchopanza Thanks for the link. I added it to my favorites for future reference. – Code-Apprentice Aug 11 '12 at 18:20
  • 4
    @Code-Guru Cleaner to read for whom? The standard C++ idiom is to define `+` in terms of `+=`; it's even possible to do this by means of derivation from an instance of a template. Deviating from the standard idiom makes code more difficult to read, because less expected. (Regardless of what one thinks of the standard idiom, and there are some I'm not too happy with.) – James Kanze Aug 11 '12 at 18:29