Operator Overloading - RHS object is modified by assignment operation

Dear cBoard,

I wrote a 3d vector class, and it works pretty well. I was hoping to overload some operators to make the class a bit easier to use.

Where * operator is over loaded to scale the vector by a scalar float.

Code:

Vec3f a, b;
//assign relevant values to a.x, a.y, and a.z
a.normalize();
b = a * CONST_FLOAT_0; //This modifies a
//b is used for something
b = a * CONST_FLOAT_1

I've never used operator overloading much. I read a couple pages about operator overloading and realized that I didn't understand references very well. I read the eternally confuzzled page about pointers and references. I won't lie it left me pretty confused, but gave me a better idea of what's going on.

The problem is very clear to me. It's doing what I expect it to, but not what I want it to.
Somehow, during an assignment operation nothing on the right side should be modified.

Here is = and *

Code:

Vec3f& Vec3f::operator=(const Vec3f& rhs)
{
    x = rhs.x;
    y = rhs.y;
    z = rhs.z;
    return *this;
}

Vec3f& Vec3f::operator*(const float& rhs)
{
    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

I think I could fix it if I understood how data is flowing in these functions.

Why would an assignment operator return a value? when operator= is called, isn't it being called by the left hand side? So what is that return value doing, seems like this function should be void to me.

I guess using references allows you to have multiple assignments on one line, I'll have to figure that one out.

When I use
b = a * float;
how can the * operator know that it shouldn't change a?
I tried creating a temporary Vec3f in operator* and then modifying it and returning it, but that didn't fix the problem.

How should these overloaded operators work?
Thank You

EDIT:
Here's the class declaration:

Code:

class Vec3f {
	public:
        	Vec3f();
        	Vec3f(float xi, float yi, float zi);
        	~Vec3f();
        	float x,y,z;

        	Vec3f operator+(Vec3f rhs);
	        Vec3f operator-(Vec3f rhs);
	        Vec3f& operator*(const float& rhs);
	        Vec3f operator/(float rhs);
	        Vec3f operator+=(Vec3f rhs);
	        Vec3f& operator=(const Vec3f& rhs);
	
	        float dot(Vec3f rhs);
	        Vec3f cross(Vec3f rhs);
	        float getMag();
	        float getXAngle();
	        float getYAngle();
	        void setAngle(float a);
	        void setAngles(float xa, float ya);
	        void normalize();
	        void print();
	protected:
	        float xAngle_;
	        float yAngle_;
};

Code:

When I use
b = a * float;
how can the * operator know that it shouldn't change a?

Actually, the operator doesn't know that. you have to tell it that "a" must be const. You can do so by putting "const" at the end of your operator. Then, when the class "a" call "operator*", "a" will be const.

Code:

Vec3f& Vec3f::operator*(const float& rhs)  const 
{
    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

When you call

Code:

b = a * CONST_FLOAT_0

it's actually, a.operator*(CONST_FLOAT_0). This will operate on A. so you will have a.x*=rhs, a.y*=rhs, a.z*=rhs. This will change the value of 'a'.

What you actually want is a new object that is a modified version of 'a'. You can do this by using the constructor to create that new object, and return it by 'value'. Since, it's a temporary object, there is no need to return by reference.

Code:

Vec3f Vec3f::operator*(const float& rhs)  const 
{
    return Vec3f(rhs * x, rhs*y, rhs*z); // using constructor to create a new temperary object.
}

For the "=" operator, since "b" will call that operator, you actually don't have to return, because x,y,z assignment will be b.x,b.y,b.z.

Code:

void Vec3f::operator=(const Vec3f& rhs)
{
    x = rhs.x;
    y = rhs.y;
    z = rhs.z;
}

I think what you didn't get was the scope when you call these methods. When you use the method, you will be in scope with object. So, whenever you call x,y,z, you will get *this.x, *this.y, and *this.z. Usually, you want to return a reference if you pass a reference value into your method, and you want to return that value.

Code:

Vect3d & somethod(Vect3d & name)
{
    /// something
    return name;
}

It doesn't make sense to return by referece if you want to return a variable created inside your function because that variable will go out of scope after the function call.

BTW, in your class declaration, you may want to declare x,y,z to be private for encapsulation.

Thanks for replying!

This helps a lot, I was thinking about it all wrong. Returning the temporary copy make sense to me. And I can see that returning a reference to it would not work.

I still don't quite understand what const does when its at the end like that. Does this mean that the function can't modify the class? And if I returned the temporary copy like you said, then there is no need to add that const?

If I made the method const, and continued to use it the way I am:

Code:

Vec3f& Vec3f::operator*(const float& rhs)  const 
{
    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

This doesn't actually do anything, right? It is ignoring the rhs, and "this" is unchanged?

Thanks for helping me

edit:
Ah, I see that when I make the function constant I can't even compile. It tells me that I'm trying to modify a read only structure. So am I correct in saying that adding const to the end of a class method prevents the method from modifying the class?

Originally Posted by IdioticCreation

Thanks for replying!

This helps a lot, I was thinking about it all wrong. Returning the temporary copy make sense to me. And I can see that returning a reference to it would not work.

I still don't quite understand what const does when its at the end like that. Does this mean that the function can't modify the class? And if I returned the temporary copy like you said, then there is no need to add that const?

If I made the method const, and continued to use it the way I am:

Code:

Vec3f& Vec3f::operator*(const float& rhs)  const 
{
    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

This doesn't actually do anything, right? It is ignoring the rhs, and "this" is unchanged?

Thanks for helping me

edit:
Ah, I see that when I make the function constant I can't even compile. It tells me that I'm trying to modify a read only structure. So am I correct in saying that adding const to the end of a class method prevents the method from modifying the class?

When you use "const" like that, it says that the calling object is read only. So when you do, a.operator*(3), "a" will be a const. However, when you go into the body of the method, what you're doing is:

Code:

a.x *=rhs;
a.y *=rhs;
a.z *=rhs;

Then you're trying to change the values of a. If you compile what you just did, the compiler will protest, because you are trying to alter a const object.

And if I returned the temporary copy like you said, then there is no need to add that const?

You don't have too. Just like function argument, you don't have to make any thing const if you sure 100% of the time you remember not to touch these variable passing into your function or methods. But that's just dreaming. It's better to use "const" so that when you accidentally try to alter these classes, you get a compiling error, which is easier to fix than going to look for something that's not supposed to happen during runtime.

Awesome, I feel much better about this now.

Originally Posted by IdioticCreation

Ah, I see that when I make the function constant I can't even compile. It tells me that I'm trying to modify a read only structure. So am I correct in saying that adding const to the end of a class method prevents the method from modifying the class?

Yes.

Incidentally, it is unusual for an operator*() to return a reference, or for it to return *this (unless multiplying an object by anything yields itself - which amounts to an unusual definition of multiplication).

Your operator*() would be better specified as an operator*=() - which would not usually be const, but would return *this.

So, for your multiplication operators, you might have.

Code:

class Vec3f
{
     public:

         Vec3f &operator*=(const float &);
         Vec3f operator*(const float &) const;   // note:  not returning a reference
};

Vec3f &Vec3f::operator*=(const float &rhs)
{
    // note that this does change  *this - the current object

    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

Vec3f Vec3f::operator*(const float &rhs) const
{
      Vec3f temp(*this);   // create a copy of self

      temp *= rhs;     //  use the operator *=() above

      return temp;
}

This means that "a *= 2.0f;" will double all the elements in a. However, "b = a*2.0f;" will not modify a, but the result of the multiplication will be stored in b.

Yes, I think I understand it quite well now, and it's working good. Here's the whole class now.

Code:

#ifndef VEC3F_H_INCLUDED
#define VEC3F_H_INCLUDED

#include <iostream>
#include <math.h>

class Vec3f {
	public:
	        Vec3f();
        	Vec3f(float xi, float yi, float zi);
	        ~Vec3f() {}
	        float x,y,z;

	        Vec3f operator=(const Vec3f& rhs);
	        Vec3f operator+(const Vec3f& rhs);
	        Vec3f operator-(const Vec3f& rhs);
	        Vec3f operator*(const float& rhs);
	        Vec3f operator/(const float& rhs);
	        Vec3f operator+=(const Vec3f& rhs);
	        Vec3f operator-=(const Vec3f& rhs);
	        Vec3f operator*=(const float& rhs);
	        Vec3f operator/=(const float& rhs);

	        float getMag();
	        void normalize();
	        float dot(Vec3f rhs);
	        Vec3f cross(Vec3f rhs);
	        float getXAngle();
	        float getYAngle();
	        void setAngles(float xa, float ya);

	        friend std::ostream& operator<< (std::ostream& stream, const Vec3f& v);
	protected:
	        float xAngle_;
	        float yAngle_;
};

#endif //VEC3F_H_INCLUDED

Code:

#include "vec3f.h"

Vec3f::Vec3f()
{
    x = y = z = 0.0;
}

Vec3f::Vec3f(float xi, float yi, float zi)
{
    x = xi;
    y = yi;
    z = zi;
}

Vec3f Vec3f::operator=(const Vec3f& rhs)
{
    x = rhs.x;
    y = rhs.y;
    z = rhs.z;
    return *this;
}

Vec3f Vec3f::operator+(const Vec3f& rhs)
{
    return Vec3f(x + rhs.x, y + rhs.y, z + rhs.z);
}

Vec3f Vec3f::operator-(const Vec3f& rhs)
{
    return Vec3f(x - rhs.x, y - rhs.y, z - rhs.z);
}

Vec3f Vec3f::operator*(const float& rhs)
{
    return Vec3f(x * rhs, y * rhs, z * rhs);
}

Vec3f Vec3f::operator/(const float& rhs)
{
    return Vec3f(x / rhs, y / rhs, z / rhs);
}

Vec3f Vec3f::operator+=(const Vec3f& rhs)
{
    x += rhs.x;
    y += rhs.y;
    z += rhs.z;
    return *this;
}

Vec3f Vec3f::operator-=(const Vec3f& rhs)
{
    x -= rhs.x;
    y -= rhs.y;
    z -= rhs.z;
    return *this;
}

Vec3f Vec3f::operator*=(const float& rhs)
{
    x *= rhs;
    y *= rhs;
    z *= rhs;
    return *this;
}

Vec3f Vec3f::operator/=(const float& rhs)
{
    x /= rhs;
    y /= rhs;
    z /= rhs;
    return *this;
}

float Vec3f::getMag()
{
    return sqrtf(x*x + y*y + z*z);
}


void Vec3f::normalize()
{
    *this /= this->getMag();
}

float Vec3f::dot(Vec3f rhs)
{
    float r = 0.0;
    r = x * rhs.x;
    r += y * rhs.y;
    r += z * rhs.z;
    return r;
}

Vec3f Vec3f::cross(Vec3f rhs)
{
    Vec3f r;
    r.x =  (this->y * rhs.z) - (this->z * rhs.y);
    r.y =-((this->x * rhs.z) - (this->z * rhs.x));
    r.z =  (this->x * rhs.y) - (this->y * rhs.x);
    return r;
}

float Vec3f::getXAngle()
{
    /*code here to calculate the angle*/
    return xAngle_;
}

float Vec3f::getYAngle()
{
    /*code here to calculate the angle*/
    return yAngle_;
}

void Vec3f::setAngles(float xa, float ya)
{
    /*See notes about this function*/
    xAngle_ = xa;
    yAngle_ = ya;
    x = -sinf(xa);
    y = tanf(ya);
    z = -cosf(xa);
    this->normalize();
}

std::ostream& operator<<(std::ostream& stream, const Vec3f& v)
{
    std::cout << "x = " << v.x << ", y = " << v.y << ", z = " << v.z;
    return stream;
}

Let me know if you have any suggestions.

Remove the declaration and definition of operator= because the compiler generated version will suffice.

The constructors can utilise the initialisation list:

Code:

Vec3f::Vec3f() : x(0.0), y(0.0), z(0.0) {}

Vec3f::Vec3f(float xi, float yi, float zi) : x(xi), y(yi), z(zi) {}

Your operator+= should return by reference, e.g.,

Code:

Vec3f& Vec3f::operator+=(const Vec3f& rhs)
{
    x += rhs.x;
    y += rhs.y;
    z += rhs.z;
    return *this;
}

The same goes for operator-=, operator*= and operator/=.

You can implement operator+ as a non-member non-friend function:

Code:

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

If you really do want to implement it as a member function then it should be declared const. Note that my comments concerning operator+ apply to operator-, operator* and operator/.

getMag, dot, cross, getXAngle and getYAngle should be const member functions. dot and cross should have const reference parameters.

Your implementation of operator<< looks wrong: you should be using the std::ostream object named stream, not std::cout.

Replace these lines in the header:

Code:

#include <iostream>
#include <math.h>

with:

Code:

#include <iosfwd>

In the source file, replace this line:

Code:

#include <math.h>

with:

Code:

#include <cmath>
#include <ostream>

The reason is that <cmath> is the C++ equivalent of <math.h> and you only need <ostream>, not <iostream> to implement the overloaded operator<<. If you later decide to overload operator>> for input, you should #include <istream> (<iosfwd> can remain for the header).

EDIT:
Oh, the member variables should be declared private. You do not need to define the destructor as the compiler generated version will suffice. You should declare xAngle_ and yAngle_ as mutable since they perform a caching function (so that you can still declare getXAngle and getYAngle as const member functions). You may need boolean flags to check whether those values have already been computed and cached.

Yay! Thanks for all the suggestions.

I intended to have += and the likes return reference. I was also wondering - say the functions took integers instead of floats. Would pass by reference be helpful still. How big is a memory address?

Would using a const member function vs a non-member non-friend function be a stylistic choice?
If I wanted to keep x, y, and z public, then using a non-member function wouldn't be helpful, right? While using a const member function would still have the desired effect?

I think I want to keep x, y, and z public because I think the code looks nicer that way. Maybe you can convince me not to?

Good idea about the flags to see if the angles have been calculated.

Thanks so much!

I was also wondering - say the functions took integers instead of floats. Would pass by reference be helpful still. How big is a memory address?

Passing by reference is helpful when you have a large class. Instead of having to create a temporary object during function call, it would be more efficient for the function to work on the object itself via the alias. Passing in variable type int or float wouldn't make that much different; and it may prevent you from using that function with literal constants.

Code:

int & func( int & a);
.
.
.
// in main:
func(1); // won't compile. But.

a = 1;
func(a); // this works.

I think I want to keep x, y, and z public because I think the code looks nicer that way. Maybe you can convince me not to?

You actually wants to data-hiding by putting these variables in private, and only alter the data through the interface or the public methods. If you don't do that, anything can alter these data; then you won't need those public methods, and any non-member function can access and use these data points (this is what you want to prevent by using private). At this point, your class behaves just like a struct. In that case, just write your code in C.

Originally Posted by nimitzhunter

Passing by reference is helpful when you have a large class. Instead of having to create a temporary object during function call, it would be more efficient for the function to work on the object itself via the alias. Passing in variable type int or float wouldn't make that much different; and it may prevent you from using that function with literal constants.

Code:

int & func( int & a);
.
.
.
// in main:
func(1); // won't compile. But.

a = 1;
func(a); // this works.

if you declared it as int& func(const& a);
then it would compile. I think a memory address is smaller than a float, so there is some advantage to be had. But yes, definitely with a class it is good.

OK, I don't mean to argue here, but I always hear that data hiding is good yet I don't really understand why. I mean, how does it make a difference? The data can still be altered, you're only changing how it is to be altered. I just don't like it because it seems obfuscating sometimes. Probably if I understood why its necessary, then I wouldn't mind it much.

You can protect yourself from invalid data in your member variables that way.

You don't only hide the data, but also implementation details.
If you only have public methods that are used, you can change the inners of the class without breaking the existing code that uses it's instances.

Maybe you wrote a Matrix class, but you thought that having a matrix hold as int** isn't the brightest of ideas, so you change it let's say to std::vector<std::vector<int> > - or you thought that emulating 2d array using 1d array would be better - you just go ahead and do that without changing anything outside the class.
Or maybe you thought that having 100000x100000 matrix that only has a few non-zero elements (very sparse one) isn't really that memory efficient with arrays/vectors, so at that point you implement it using a, let's say, std::map, and maybe let the object decide how the data is hold based on the data itself.

Originally Posted by IdioticCreation

then it would compile. I think a memory address is smaller than a float, so there is some advantage to be had. But yes, definitely with a class it is good.

It's actually system dependent (or, in the language of the C++ standard, implementation defined). However, a typical float is 4 bytes and on a 32-bit system a pointer is typically 4 bytes (8 bit per character, by 4).

Size, however, is not the only factor to consider. Practical classes often have either non-trivial constructors and destructor or contain objects with non-trivial constructors and destructors. A class that only contains a pointer, for example, is pretty small (eg sizeof() yields 4 on a lot of 32 bit system), but the class may be managing dynamically allocated memory or some other limited system resource (eg file handles). Copying class objects is therefore often relatively expensive.

Also bear in mind it doesn't make sense to copy all objects. The technique of "private copy constructor that is not implemented" prevents passing an object by value. However, it is still possible to pass such an object by const reference.

As a rule of thumb, however, I pass all basic types (char, int, float, double, pointers) by value rather than const reference, and pass all struct/class types by reference. The only way I'd change away from that would be if was absolutely necessary (eg to address performance bottlenecks found in profiling). I've yet to find a practical case where that was necessary.

Thanks for the info grumpy, this is really helpful

Originally Posted by grumpy

Also bear in mind it doesn't make sense to copy all objects. The technique of "private copy constructor that is not implemented" prevents passing an object by value. However, it is still possible to pass such an object by const reference.

I'm afraid I don't follow you here, sorry you'll have to explain your joke.

I understand what your saying Xupicor. So if I make x, y and z private add getters for them, then if I wanted to use an array of floats for x, y, z instead of three separate variables, I could do this and to the user the class hasn't changed. This example is very trivial, but I can see how it becomes more important.

I went ahead and made x, y, and z private. I was able to get away without needed to write setX, setY and setZ. I guess this is a set in the right direction for protecting data? Now they can only be modified by indirect methods...and the assignment method.

If I did write setters then the data doesn't seem any safer than it was before, is that not the case?

Originally Posted by IdioticCreation

Thanks for the info grumpy, this is really helpful



I'm afraid I don't follow you here, sorry you'll have to explain your joke.

The usual example are streams (like cout, cin, file streams, etc.) You don't want to be making a copy of cin or cout, because that doesn't really make a lot of sense. So all the input/output functions work with (and return) references. (In this case they can't be const references, because the state of cin/cout will change, but we still want to disable copying.)

Originally Posted by IdioticCreation

If I did write setters then the data doesn't seem any safer than it was before, is that not the case?

Then you didn't write the setters appropriately -- or, more likely in this case, you don't have any rules to enforce.

Code:

void my_class::setMinute(int min) {
    if (min < 0 || min > 60) {
        Administer_Electric_Shock_To_User();
    } else {
        internal_minute = min;
    }
}