C++ adding 2 3D vectors returns garbage value

Question

I am very new to c++ and just started to learn operator overloading. Here is my attempt

class Vector
{
 public:
 float x=0,y=0,z=0;
 Vector(float x, float y, float z) :x(x),y(y),z(z) {}
 Vector(Vector& copy) :x(copy.x),y(copy.y),z(copy.z){ std::cout << "Copy Created" << std::endl;} //Testing if any new objects were created during the operator overloading process[For my purpose this should not be called as no new objects should be created except than the returned result of each operator]

 public:
 Vector& operator+(Vector& v1) //return this+v1
 {
     Vector v(this->x+v1.x,this->y+v1.y,this->z+v1.z);
     return v;
 }
 Vector& operator-(Vector& v1) //return this-v1
 {
     Vector v(this->x - v1.x, this->y - v1.y, this->z - v1.z);
     return v;
 }
 Vector& operator*(Vector& v1) //return this cross v1
 {
     Vector v(this->y * v1.z-this->z * v1.y, -this->x * v1.z + this->z * v1.x, this->x * v1.y - this->y * v1.x);
     return v;
 }
}

std::ostream& operator<<(std::ostream& output, Vector& v)  
{
    output << v.x << "," << v.y << "," << v.z << std::endl;
    return output;
}

int main()
{
    Vector
    v1(1, 2, 3),
    v2(4, 5, 6);

    Vector
    v3 = v1 + v2,
    v4 = v1 - v2,
    v5 = v1 * v2;
  
    std::cout << v3 << v4 << v5;

  return 1;
}

All 3 vectors when printed have garbage values and the copy constructor was invoked 3 times for each operation. I have passed each of the vectors by reference but still an new temporary instance was created somewhere i don't understand.

I have also tried adding the keyword const to both the operators and their parameters as was suggested by previous threads but it didn't solve the problem

Since i am a novice an elaborate explanation of the solution would greatly be appreciated. Thank you

If you turn up warnings on your compiler, it should tell you that you are returning references to temporary (local) variables. Don't do that, return copies instead. — Botje, Oct 22 '20 at 14:01
operators `+`, `*` and `-` should return a value not a reference. see here: https://stackoverflow.com/questions/4421706/what-are-the-basic-rules-and-idioms-for-operator-overloading — 463035818_is_not_an_ai, Oct 22 '20 at 14:03
(note that it is "should", its the conventional way, in principle those operators can return anything, but returning a reference to a local is wrong in any case) — 463035818_is_not_an_ai, Oct 22 '20 at 14:04
@Botje you were right i did turn on the warnings of my compiler and it did show me the warnings but it dissapeared immediatly when the output showed up so i coudn't see it :) — Sync it, Oct 22 '20 at 14:39
@idclev 463035818 do you know how to set that flag in visual studio 2019? — Sync it, Oct 22 '20 at 14:53

463035818_is_not_an_ai · Accepted Answer · 2020-10-22T15:27:58.537

Operators +, * and - conventionally return a copy not a reference:

Vector operator+(const Vector& v1) const
  //^^ no &
                //^^ added const  
                                  //^^ added const
{
   Vector v(this->x+v1.x,this->y+v1.y,this->z+v1.z);
   return v;
}

Also you should declare the method as const, so you can call operator+ on a constant vector. And the paramter should be taken as const reference.

For more details on operator overloading see here: What are the basic rules and idioms for operator overloading?

In your code you are returning a reference to a local variable which is wrong always. A simplified example is

int& foo() {
   int x = 0;
   return x;
}

The life time of the local variable x ends when the function returns and the caller gets a dangling reference. Using that reference invokes undefined behavior.

If you do want to avoid the copies you should overload the compound operators +=, *=, -=. They are expected to perform the operation in-place and typically return a reference to this after modifying it. For details see the above link.

I wrote "should", "typically" and "conventionally" above. Operator overloading is rather flexible and can do the most weirdest things. However, convention is to return a fresh value from operators +, * and -, and returning a reference to a local variable is always wrong.

Last but not least, I want to mention that it is refreshing to see a beginners code with so little bad practices applied. Apart from your error, the only thing I would critisize is const correctness. Make methods and parameters const by default. Only when you need to modify them make them non-const. For example your operator<< should also take a const Vector& because it does not modify it.

Thank you for the beginner tips. It turns adding const everywhere including in the "COPY CONSTRUCTOR" itself Vector(const Vector& v) finally fixed the problem. I shall do more research on the const modifier to learn better practices — Sync it, Oct 22 '20 at 15:44
@RVISHAL no adding `const` alone will not fix the problem! Note that undefined behavior means that your code is wrong, but not necessarily you get any warning or error during compilation. When you run code that has UB it can appear to work, but it is still wrong. You need to fix returning the reference to the local variable! — 463035818_is_not_an_ai, Oct 22 '20 at 15:45
@RVISHAL the missing `const` would create problems in other code, not in yours (unless I miss something) — 463035818_is_not_an_ai, Oct 22 '20 at 15:46
i have changed my code. I remove returning references to just returning values but i don't want to edit my question so that hopefully someone in the future looking at this post will understand my mistakes and will correct it themselves by reading your answer — Sync it, Oct 22 '20 at 15:48
@RVISHAL thats the right way. You should never fix the problems in your code in the question (unless it was something completely unrelated to the actual question). Questions are for the broken code, fixed code goes to answers. — 463035818_is_not_an_ai, Oct 22 '20 at 15:50

ytlu · Answer 2 · 2020-10-22T18:24:39.843

According to your code, I suggest to change you operator to operator+= -= and *=. They are much fitted to your purpose. At the end of each operation, return *this. It means returning the current working object. For example:

vector& vector::operator+= (const vector& v) 
 { this->x += v.x;
   this->y += v.y;
   this->z += v.z;
   return *this; 
 }

and write operator+ as a two arguments friend function:

 vector operator+(const vector&v1, const vector&v2)
  { vector v3(v1);  // copy constructor
    v3 += v2;
    return v3;
  }

score 0 · Answer 3 · answered Oct 22 '20 at 14:17

0

Not sure but I think you getting garbage values because you trying to return reference to local stack value v. After end of scope of you overloaded operators the 'v' value may be overridden by another part of program.

Possible solution is:

Vector& operator+(Vector& v1) //return this+v1
{
    return *new Vector(this->x+v1.x,this->y+v1.y,this->z+v1.z);
}

answered Oct 22 '20 at 14:17

Nikita Mirošničenko

197
8

1

Thank you that worked for me. I looked up a tutorial about dynamic vs stack memory just now after seeing your answer and i understood a lot. Every small tutorial site never suggested this and they would wrongly show an screen shot of the correct output and it had me stumped for a while. devil in the details i guess :) – Sync it Oct 22 '20 at 14:36
this looks like a recipe for lots of memory leaks. Who is going to `delete` that `Vector`? Thinks will get super complicated when you call `operator+` on a stack allocated `Vector` because then you need to keep track of which `Vector` was newed and which was not manually. – 463035818_is_not_an_ai Oct 22 '20 at 14:39
this is not a good solution. It makes OPs code compile without errors, but it will lead to problems – 463035818_is_not_an_ai Oct 22 '20 at 14:39
Well the answer did work for me but now after reading your comments should i unmark this as the accepted answer? Is there an better alternative? – Sync it Oct 22 '20 at 14:49
Tastes quite a bit perverted but... still works nonetheless :) – Netherwire Oct 22 '20 at 15:25
@RVISHAL you can for example try `for (int i = 0 ; i < 100000; ++i) { Vector a,b; auto c = a +b; }` to watch your memory usage growing and you have no way to free the memory after that loop. This solution is bad. – 463035818_is_not_an_ai Oct 22 '20 at 15:26

C++ adding 2 3D vectors returns garbage value

3 Answers3