C++ adding 2 3D vectors returns garbage value

user_5794313 0
Viewed 143

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

copy-constructor operator-overloading c++
3 Answers

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.

user_41177280

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;
  }
user_144864190

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);
}
user_42566870
Related