Overloaded Assignment Operator Quick Question
Author
Hi all,
Can somebody please tell me why this overloaded assignment operator...:
inline MY_MODEL * MY_MODEL::operator= ( MY_MODEL * My_Model )
{
this = My_Model;
return this;
};
...produces the following error about the "this" pointer:
error C2106: ''='' : left operand must be l-value
...I''m having trouble getting to grips with this whole overloaded operator thing - I can see it''s very useful, but can''t quite grasp the syntax (particularly for this assignment operator!).
Any help on the topic will be appreciated!
Thanks,
Gareth
Ok, firstly, your syntax is ''wrong''. The assignment operator should generally return a reference (to the object it was called upon) and takes a reference to another object as a parameter. You could have it take a pointer as a parameter, as you are doing here, but that is generally not what is desired or required.
But your main problem is that you are trying to change the ''this'' pointer, and you can''t do that. The idea of the assignment operator is not to ''move'' the object you are working with, it''s to make sure that the value of the object you are working with is equal to the value of the object you are passing in. The actual error you are getting is saying that you can''t change the ''this'' pointer, because the ''this'' pointer is not an L-value. An L-value is a value that can be assigned to.
So it should probably look more like this:
But your main problem is that you are trying to change the ''this'' pointer, and you can''t do that. The idea of the assignment operator is not to ''move'' the object you are working with, it''s to make sure that the value of the object you are working with is equal to the value of the object you are passing in. The actual error you are getting is saying that you can''t change the ''this'' pointer, because the ''this'' pointer is not an L-value. An L-value is a value that can be assigned to.
So it should probably look more like this:
inline MY_MODEL& MY_MODEL::operator= (MY_MODEL& My_Model ){// Copy internal data across so that they matchwhateverData = My_Model.whateverData;// Dereference a pointer, return it (will be returned as a reference)return *this;};
Actually, do it like so:
inline MY_MODEL& MY_MODEL::operator= (MY_MODEL& My_Model )
{
if (this != &My_Model) {
// copy field by field from My_Model to this
}
return *this;
}
Copying an object to itself should be checked for and stopped, which the above code will do, but also at times the assignment operator code is written with the assumption that there are two distinct MY_MODEL objects. Consider the following:
delete this->someField;
this->someField = new SomeObject(My_Model.someField);
Since this == &My_Model the first line clears the object at someField but it doesn''t set the pointer to null. In the next line when My_Model.someField is accessed you''re pointing off into no-mans land.
inline MY_MODEL& MY_MODEL::operator= (MY_MODEL& My_Model )
{
if (this != &My_Model) {
// copy field by field from My_Model to this
}
return *this;
}
Copying an object to itself should be checked for and stopped, which the above code will do, but also at times the assignment operator code is written with the assumption that there are two distinct MY_MODEL objects. Consider the following:
delete this->someField;
this->someField = new SomeObject(My_Model.someField);
Since this == &My_Model the first line clears the object at someField but it doesn''t set the pointer to null. In the next line when My_Model.someField is accessed you''re pointing off into no-mans land.
Author
Great, thanks for your help both of you. I''ll go and sort out my code now.
Thanks,
Gareth
Thanks,
Gareth
The "correct" way to write an assignment operator is to pass the object by value, not reference
MY_MODEL& MY_MODEL::operator=( MY_MODEL My_model )
{
// swap the data in My_model (must be non throwing)
return *this;
}
Or if you must pass by reference
MY_MODEL& MY_MODEL::operator=( MY_MODEL &my_model )
{
MY_MODEL temp( my_model );
// swap data with temp (must be non throwing)
return *this;
}
This is to implement assignment op in terms of the copy ctor. The copy is necessary to achieve exception safety. Take a look at the first few tips of Guru of the Week (www.gotw.ca) for more insight or get the book "Exceptional C++" by Herb Stutter
The test
if (test == &my_model)
is not necessary and is only an optimization. It has problems because the class can override the == and & operator, and the above test will be screwed.
So take the safe path, and copy by value.
MY_MODEL& MY_MODEL::operator=( MY_MODEL My_model )
{
// swap the data in My_model (must be non throwing)
return *this;
}
Or if you must pass by reference
MY_MODEL& MY_MODEL::operator=( MY_MODEL &my_model )
{
MY_MODEL temp( my_model );
// swap data with temp (must be non throwing)
return *this;
}
This is to implement assignment op in terms of the copy ctor. The copy is necessary to achieve exception safety. Take a look at the first few tips of Guru of the Week (www.gotw.ca) for more insight or get the book "Exceptional C++" by Herb Stutter
The test
if (test == &my_model)
is not necessary and is only an optimization. It has problems because the class can override the == and & operator, and the above test will be screwed.
So take the safe path, and copy by value.
Well, there is ''correct'' in terms of ''strongly execption-safe'' and ''correct'' in terms of ''not taking more copies than you need''. If you''re not using exceptions in your code, doing it that way creates unnecessary temporaries and gains you nothing. In fact, if you use the ''swapping the temporary with the current object'' method, you also do twice as many assignments as you need to. So, whether that is right or wrong all depends on whether you''re using exceptions or not. If you''re not using exceptions, you''re best off doing it the efficient way.
As stated on GOTW #11:
"a) if you can test for self-assignment then you can completely optimize away the assignment; and b) often making code exception-safe also makes it less efficient (a.k.a. the "paranoia has a price principle".
As stated on GOTW #11:
"a) if you can test for self-assignment then you can completely optimize away the assignment; and b) often making code exception-safe also makes it less efficient (a.k.a. the "paranoia has a price principle".
I agree with your point on inefficiency but I can only imagine code that does not guarantee even basic exception safety are written by amatuers or in platforms with so limited memory that you have to worry about every byte allocation.
If one disregard exception issues, the code is prone to leaks (memory & resource) when an exception occurs.
Juz because just about every window application leaks when they crash doesn''t mean we have to accept it as a way of life.
If making a copy is expensive, consider reference counting or the flyweight pattern.
I prefer correctness over speed.
If one disregard exception issues, the code is prone to leaks (memory & resource) when an exception occurs.
Juz because just about every window application leaks when they crash doesn''t mean we have to accept it as a way of life.
If making a copy is expensive, consider reference counting or the flyweight pattern.
I prefer correctness over speed.
Void, whether it''s ''right'' or not, you have to accept the fact that 99% of code written by people on these forums does not use exceptions. I expect no more than 10% of people are even using the STL. And iostreams only throw when you ask them to. So the only exceptions they''re ever likely to need to handle is bad_alloc... and Visual C++, the compiler of choice for most people here, doesn''t even throw bad_alloc by default, it returns NULL from new. (Yes, I know this is non-standard.) So expecting everyone to spend a lot of time making code strongly exception safe (and let''s face it, if making code strongly exception safe wasn''t a very difficult task, there wouldn''t be Guru Of The Week columns on it) when their code doesn''t throw any exceptions is a bit pointless.
I''ve usually done assignment as follows -
MyClass& MyClass::operator=(const MyClass& rhs)
{
if ( this != &rhs )
{
// acquire new resources
// free old resources
// assign in new resources to this
}
return *this;
}
MyClass& MyClass::operator=(const MyClass& rhs)
{
if ( this != &rhs )
{
// acquire new resources
// free old resources
// assign in new resources to this
}
return *this;
}
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
