tags:

views:

189

answers:

8

Hi, I am trying to override the = operator so that I can change my Point class into a Vector3 class.

Point tp = p2 - p1;
Vec3 v;
v = tp;

The problem I am facing is that, "v" will have its x,y,z members equal to zero all the time.

Vec3.h:

Vec3 operator =(Point a) const;

Vec3.cpp:

Vec3 Vec3::operator =(Point a) const
    {
        return Vec3(a.x,a.y,a.z);
    }

Thanks for all the help once again :)

+1  A: 

Assignment operator work like this.

Vec3.h:

Vec3& operator = (const Point &a);

Vec3.cpp:

Vec3& Vec3::operator = (const Point &a)
{
    x = a.x;
    y = a.y;
    z = a.z;
    return *this;
}

Notice that you're modifying this object and return a non-const reference to it.

Keynslug
Beat me to it, but ditch the const now, because you modify the object.
Eloff
@Eloff: He's *not* modifying `a`...?
Mark
Eloff
+1  A: 

You want this:

Vec3 & Vec3::operator =(const Point &a) 
{
x = a.x;
y = a.y;
z = a.z;
return *this;
}
  1. Assignment should modify the this object, not return something
  2. Return a reference to object just modified
Winston Ewert
You're missing a semicolon. You should change the line to "z=b.z ;"
Dragontamer5788
Input parameter should be a const reference, and not passed by value. (From the Committee to Stop Stack Abuse and Excessive Copies. ^_-)
Mike DeSimone
+4  A: 

It's been a while, but I think you want

Vec3& Vec3::operator=(const Point &a) 
{
    x = a.x; y = a.y; z = a.z;

    return *this;  // Return a reference to myself.
}

Assignment modifies 'this', so it can't be const. It doesn't return a new Vec3, it modifies an existing one. You will also probably want a copy constructor from Point, that does the same.

Russell Borogove
IIRC, there's a trinity here somewhere. It says that if you override `operator=`, you also need to put in a copy constructor and override one other thing... and I'm blanking on it.
Mike DeSimone
@Mike: That's only if the class manages resources. It's The Rule of Three. And you use the copy-and-swap idiom to implement it. It isn't needed here, though implementing this is surely an indicator of bad design.
GMan
Yeah I am still new to this business :) Thanks for all the help
Aero
A: 

Why not just do this:

void Vec3::SetCoord(const Point& pnt)
{
    x = pnt.x;
    y = pnt.y;
    z = pnt.z;
}

OR...

Make a new overloaded constructor:

Vec3::Vec3(const Point& pnt)
{
    x = pnt.x;
    y = pnt.y;
    z = pnt.z;
}

No crazy casting or equals operator involved. I'd recommend doing one of these ways, its probably the easiest to maintain.


To get the equals-syntax above should really add a type conversion operator, but to Point:

class Point
{
     // ....

     operator Vec3()
     {
        return Vec3(this->x,this->y,this->z);
     }
};

Doing an assignment between the two objects like that is kinda funny without assuming a type-conversion. Most classes don't behave that way syntactically.

Note, though, that this can cause some ambiguity, and a number of well-respected folks on this site who answer questions on C++ would say the type-conversion answer can cause all sorts of nasty issues. So I'd avoid it.

sheepsimulator
The conversion operator should be const-qualified (though, I'd be very wary of using a conversion operator; if you want an implicit conversion, a converting constructor is usually less error-prone).
James McNellis
@James - Concur.
sheepsimulator
A: 

There are some answers explaining how to do what you want, but I will explain what's going on with your original code:

In C++ (as in C) your assignment v = tp is an expression, not a statement. If you write x = 1 then you can write y = (x = 1) because the assignment (x = 1) is an expression with the value 1. When you override the assignment operator in C++, you are responsible for both parts: You must modify this and you must return the value of the assignment expression. Your original version only took care of returning the value of the expression. If you had written Vec3 w = (v = tp) then w would have had the expected value, while v would still be all 0.

The other way you can express the same thing in C++ is to add an operator Vec3 to your Point class so that you can use a Point anywhere you could use a Vec3. Even better is probably to have one class Vec3 and typedef Vec3 Point because they are largely identical.

Ben Jackson
+1  A: 

I agree with sheepsimulator in the fact that copy assignment operator should have the same behavior than copy constructor has. According with HIGH·INTEGRITY C++ CODING STANDARD MANUAL, you should implement an explicit conversion operator:

class Point { explicit operator Vec3() { return Vec3(this->x,this->y,this->z);  } };
ArceBrito
+1  A: 

It's more common to do this with a conversion constructor:

Vec3(const Point& p) : x(p.x), y(p.y), z(p.z) {}

This will also allow the assignment that you want.

Mike Seymour
A: 

Hmm, 6 answers here already and you've already selected one, but the simplest and most obvious IMHO is missing.

Vec3 &Vec3::operator =(Point a) // not const, return by reference
    {
        return *this = Vec3(a.x,a.y,a.z);
    }

That's all! Just insert *this = and return by reference! Done!

I would recommend implementing a conversion constructor instead, though.

Potatoswatter