Assignment operator self-assignment check

[Chris]

Is there ever a reason to declare this as

if(*this == rhs)

as opposed to what I normally see

if(this == &rhs)

?

Seems like the former version is going to be more expensive rather than
simply comparing addresses as in the latter (plus the former requires
the class to define operator== as well, no?). Wondering if that added
effort is ever justified.

[Phlip]

Chris wrote:

Is there ever a reason to declare this as

if(*this == rhs)

as opposed to what I normally see

if(this == &rhs)

?

The self-assignment check detects object identity, which C++ specifies as an
object's addresses.

The degenerate situation of copying a value into an object which already had
that value is less important. Just let it happen; don't always waste CPU
cycles checking for it.

--
Phlip
http://www.greencheese.us/ZeekLand <-- NOT a blog!!!

[Thomas Tutone]

Chris wrote:

Is there ever a reason to declare this as

if(*this == rhs)

as opposed to what I normally see

if(this == &rhs)

?

Seems like the former version is going to be more expensive rather than
simply comparing addresses as in the latter (plus the former requires
the class to define operator== as well, no?). Wondering if that added
effort is ever justified.

Well, as long as we're dealing with hypotheticals: If (1) checking the
class for equality is cheap, (2) checking for equality frequently
evaluates to true even though it's not the same object (in the sense of
occuping the same address), and (3) using the copy constructor is very
expensive, then one can imagine using the former version, since it will
avoid unnecessary copy constructions.

The latter version ought to be the default, though.

Best regards,

Tom

[Frederick Gotham]

Chris posted:

Is there ever a reason to declare this as

if(*this == rhs)

That is not a declaration -- choose your words carefully.

as opposed to what I normally see

if(this == &rhs)

The latter compares the ADDRESSES of two objects, while the former compares
the two objects themselves.

Seems like the former version is going to be more expensive rather than
simply comparing addresses as in the latter (plus the former requires
the class to define operator== as well, no?).

Yes, the former requires an accessible operator==.

Wondering if that added effort is ever justified.

Depends on a wonderous amount of factors:
(1) How the class is implemented
(2) How expensive it is to copy it
(3) How expensive it is to compare it for equality
(4) How often an object is compared to itself

If an object of the class should NEVER be assigned to itself, I would
suggest an assert:

#include <cassert>

MyClass &MyClass::opera tor=(MyClass const &rhs)
{
assert(this != &rhs);

/* Now perform assignment */

return *this;
}

--

Frederick Gotham

[S S]

Frederick Gotham wrote:

Chris posted:

Is there ever a reason to declare this as

if(*this == rhs)

That is not a declaration -- choose your words carefully.

as opposed to what I normally see

if(this == &rhs)

The latter compares the ADDRESSES of two objects, while the former compares
the two objects themselves.

Seems like the former version is going to be more expensive rather than
simply comparing addresses as in the latter (plus the former requires
the class to define operator== as well, no?).

Yes, the former requires an accessible operator==.

Wondering if that added effort is ever justified.

Depends on a wonderous amount of factors:
(1) How the class is implemented
(2) How expensive it is to copy it
(3) How expensive it is to compare it for equality
(4) How often an object is compared to itself

If an object of the class should NEVER be assigned to itself, I would
suggest an assert:

#include <cassert>

MyClass &MyClass::opera tor=(MyClass const &rhs)
{
assert(this != &rhs);

/* Now perform assignment */

return *this;
}

I think assert should not be good solution as we may not want to assert
just in case someone did it by mistake.

Here we can use shared pointers

class foo {
private:
std::tr1::share d_ptr<type_clas spt;
};

foo& foo::operator=( const foo& rhs)
{
pt.reset(new type_class(*rhs .pt)); //reset deletes the first and points
to 2nd,
//if new throws exception reset will not implement and original will
not be deleted.
return *this;
}

OR the other way

foo& foo::operator=( const foo& rhs)
{
type_class *pBefore = pt;
pt = new type_class(rhs. pt);
delete pt;
return *this;
}

These approaches are better as they are taking care of self assignment
saftey as well as exception saftey.

-SS

--

Frederick Gotham

[Noah Roberts]

S S wrote:

I think assert should not be good solution as we may not want to assert
just in case someone did it by mistake.

Well, an assert should definately not be used here but not for that
reason. Self assignment is rather common for objects when a program
gets remotely interesting. Simply not doing the assignment or devising
a way that it will just not hurt anything are the correct options.

Now, your reasoning isn't right because an assert is placed in code for
exactly that reason...in case someone accidentally violates a
pre-condition. They are there for the developer and go away in release
mode.

What you don't want is an assert that is the sole way of avoiding a
situation unless such a situation cannot be resolved, as is not the
case here. So the assert would be ok if you wanted to impose such a
restriction but the normal checks should still apply in release mode
because this could happen under conditions that where not tested and
then the program would do stupid things...and the situation is very
resolvable, you just don't do the copy.

[Frederick Gotham]

S S posted:

I think assert should not be good solution as we may not want to assert
just in case someone did it by mistake.

An assert is the testing of a condition at runtime. If the condition is
true, nothing happens. If the condition is false:

(1) The program is terminated.
(2) The programmer is informed as to where they made their coding
error.

So you see, "assert" is used to catch programming errors quickly and to cut
down time on debugging.

Here we can use shared pointers

Overkill in my opinion.
Noah Roberts posted:

Well, an assert should definately not be used here but not for that
reason. Self assignment is rather common for objects when a program
gets remotely interesting. Simply not doing the assignment or devising
a way that it will just not hurt anything are the correct options.

If the inventor of the class deems that an object of the class should never
be assigned to itself, then an "assert" is quite appropriate. The situation
is quite akin to the following:

/* Function: CountOcc

This function counts the amount of occurrences
of a specific element in an array.

NB: (1) Neither argument may be a null pointer.
(2) The second pointer must be greater than the first.
*/

template<class T>
unsigned CountOcc(T const *pstart,T const*const pend,T const &elem)
{
assert(pstart); assert(pend); assert(pend pstart);

/* Rest of Code */
}

These asserts are very good practise in my opinion. The help the developer
without sacrificing speed of execution.

--

Frederick Gotham

[Noah Roberts]

Frederick Gotham wrote:

Noah Roberts posted:

Well, an assert should definately not be used here but not for that
reason. Self assignment is rather common for objects when a program
gets remotely interesting. Simply not doing the assignment or devising
a way that it will just not hurt anything are the correct options.

If the inventor of the class deems that an object of the class should never
be assigned to itself, then an "assert" is quite appropriate.

If such an inventor did indeed deem that such a pre-condition be the
case. However, that is an inappropriate precondition to enforce and
renders the class hardly usable. When designing the assignment
operator one should always be aware of, and account for (not disallow),
self-assignment. You should also try to at least provide the strong
guarantee. Both of these can often be killed with one stone simply by
accepting the parameter by value instead of by reference.

The situation

is quite akin to the following:

/* Function: CountOcc

This function counts the amount of occurrences
of a specific element in an array.

NB: (1) Neither argument may be a null pointer.
(2) The second pointer must be greater than the first.
*/

template<class T>
unsigned CountOcc(T const *pstart,T const*const pend,T const &elem)
{
assert(pstart); assert(pend); assert(pend pstart);

/* Rest of Code */
}

These asserts are very good practise in my opinion.

The first two probably, and notice how this differs a great deal from
the self assignment problem. Namely that at least the first two
pre-conditions are totally unresolvable; there is no correct answer if
those preconditions are not met whereas the answer to self assignment
is the object itself. The last isn't even valid in that the result of
the count of any specific element in an empty array should be 0, not a
blown assert...it also guarantees nothing about the validity of that
relationship between the two addresses.

Also interesting to note is that your sticking to C constructs has
resulted in a function that is not as generic as it could be. The use
of the iterator concept instead of pointers would result in a more
useful function (it could work with any container) and remove the
necissity, and in fact the possibility, of those asserts.

The fact that asserting that a ptr != 0 in many cases is rather useless
also in that the assert can pass without a valid pointer and a 0
pointer is the easiest invalid pointer to debug. Placing the assert to
advertize the precondition is enough for it to be there though.

[Gavin Deane]

Chris wrote:

Is there ever a reason to declare this as

if(*this == rhs)

as opposed to what I normally see

if(this == &rhs)

?

Seems like the former version is going to be more expensive rather than
simply comparing addresses as in the latter (plus the former requires
the class to define operator== as well, no?). Wondering if that added
effort is ever justified.

Why are you testing for self-assignment at all? What does the rest of
your assignment operator look like? Can you implement your operator= as
"create a temporary and swap" as in http://www.gotw.ca/gotw/059.htm.

Gavin Deane

[Frederick Gotham]

Noah Roberts posted:

If such an inventor did indeed deem that such a pre-condition be the
case. However, that is an inappropriate precondition to enforce and
renders the class hardly usable.

I would say that that depends entirely on the kind of class we're
dealing with. Let's say we have a card game, and that we have a class
called "Hand". Let's say that we code the game in such a way that a hand
should never be compared to itself -- in fact, let's say that it would be
quite perverse if it were. In such circumstances, the "assert" solves two
problems:

(1) It informs the programmer of their coding error in Debug Mode.
(2) It doesn't burdeon the valid code in Release Mode with a decrease
in speed of execution.

When designing the assignment operator one should always be aware of,
and account for (not disallow), self-assignment.

I believe that rule is too rigid. I don't keep many rules in mind when
programming, I like to be fluidic and open to all possibilities -- it
results in more creative code (which tends to run faster too).

You should also try to at least provide the strong guarantee. Both of
these can often be killed with one stone simply by accepting the
parameter by value instead of by reference.

If speed of execution and memory consumption are no object, then yes.
<snip code>

>These asserts are very good practise in my opinion.

The first two probably, and notice how this differs a great deal from
the self assignment problem.

I believe they are very similar: They deal with something that the 3rd
party has outlawed. In the previous example, the 3rd party outlawed self-
assignment. In _this_ example, the 3rd party outlaws the passing of invalid
pointers.

Namely that at least the first two pre-conditions are totally
unresolvable; there is no correct answer if those preconditions are not
met whereas the answer to self assignment is the object itself.

Yes, but speed of execution suffers if self-assignment should trully be a
no-no. If it's extremely bizarre that an object of a certain type be
compared to itself, then we can just outlaw the practise by using an
"assert". If however it's not too weird that an object of a certain type be
compared to itself, then just go with:

if(this==&rhs)r eturn*this;

The last isn't even valid in that the result of the count of any
specific element in an empty array should be 0, not a blown assert...

This depends on whether the 3rd party considers an empty array to be an
array.

it also guarantees nothing about the validity of that relationship
between the two addresses.

Acknowledged, however it does guarantee that "pend" is ahead of "pstart"
(assuming of course that their comparisson doesn't invoke UB ; ) ).

Also interesting to note is that your sticking to C constructs has
resulted in a function that is not as generic as it could be. The use
of the iterator concept instead of pointers would result in a more
useful function (it could work with any container) and remove the
necissity, and in fact the possibility, of those asserts.

I have yet to read up on iterators; can you suggest a good book? I tried
TC++PL but didn't like the way it approached the Standard Library.

The fact that asserting that a ptr != 0 in many cases is rather useless
also in that the assert can pass without a valid pointer and a 0
pointer is the easiest invalid pointer to debug. Placing the assert to
advertize the precondition is enough for it to be there though.

Sorry I don't quite understand what you're saying in that last sentence.

--

Frederick Gotham