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]

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.

 

[Thomas Tutone]

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:perator=(MyClass const &rhs)
{
assert(this != &rhs);

/* Now perform assignment */

return *this;
}

 

[S S]

Chris posted:



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




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




Yes, the former requires an accessible operator==.




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:perator=(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::shared_ptr<type_class> pt;
};

foo& foo:perator=(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:perator=(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

 

[Noah Roberts]

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.

 

[Noah Roberts]

Noah Roberts posted:


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]

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.

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)return*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.

 

[Noah Roberts]

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.

Well, if it truely is not recoverable then #2 is true, but not
otherwise. If you can recover then the check needs to happen anyway.
Such is the case in self assignment and as you'll see when you go back
and read my original reply that is the primary reason an assert here is
invalid.

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).

Yes, I know that you don't follow a lot of the procedures used by
professionally minded programmers, such as adhering to coding
standards. This is a bad thing actually but it may be a while before
you realize this.

If speed of execution and memory consumption are no object, then yes.

If you say so. I think you should go do some more reading and testing
before making that assumption.

I believe they are very similar: They deal with something that the 3rd
party has outlawed.

First: What third party? Are you bringing someone new into the
argument?

And if you don't see the difference then you got problems.

Yes, but speed of execution suffers if self-assignment should trully be a
no-no.

Does it? By how much?

If it's extremely bizarre that an object of a certain type be
compared to itself,

Anyone that has ever worked on a long term and/or large project in a
team understands that such assumptions are bad assumptions to make.

Besides, we are talking assignment, not comparison.

But let's look at your example with a card hand. A card hard should
always be equal to itself no? Is there any condition in which it
wouldn't be? So why is it so perverse that you compare it to itself?
What are the concequences if it is? With an assert and no accounting
for the possibility in release mode the concequence is that such a
check could make it into production code and do unexpected things (not
so much a possibilty with comparisons but a major problem with
assignment). If the situation is just accounted for then it is just
never a problem...less debugging and no chance for post-release bug
having to do with this kind of operation.

So, the profile had better show some significant and profound impact in
the check to want to remove it since the cost in productivity and risk
of release bugs is so much higher.

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

Who is this third party you refer to?

Such policy should be enforced at a higher level. Enforcing it at this
level requires code duplication if such a function could be used on an
empty array.

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

So what?

And there is no possibily for UB here.

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 C++ Standard Library: a tutorial and reference
The C++ Standard

 

[Old Wolf]

Is there ever a reason to declare this as

if(*this == rhs)

That's a statement, not a declaration.

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.

You're missing the point of the check. This check is to prevent
assigning an object to itself.

For example, suppose the code is something like:
Type:perator=( Type const &old )
{
m_vec.clear();
copy( old.m_vec.begin(), old.m_vec.end(), back_inserter(m_vec) );
}

That code will just delete the vector's contents if you try assigning
an object to itself. Hence the simple, inexpensive check of address.

 

[Kai-Uwe Bux]

Why are you testing for self-assignment at all? What does the rest of
your assignment operator look like?

Before class specific optimization, it could generically look like this:

dummy & operator= ( dummy const & rhs ) {
if ( this != &rhs ) {
this->~dummy();
new ( this ) dummy ( rhs );
}
return ( *this );
}

Can you implement your operator= as "create a temporary and swap" as in
http://www.gotw.ca/gotw/059.htm.

Well, that would be

dummy & operator= ( dummy rhs ) {
swap( *this, rhs );
return ( *this );
}

which also calls a destructor and a copy constructor. Just the order is
different. On top of that, it calls swap, which usually is not very
expensive. The main advantage is that it provides the strong exception
safety guarantee.

Both generic solutions, however, can be prohibitively expensive. Consider
for instance the std::vector template. In this case, you would not want to
create a complete copy someplace elsewhere in memory when maybe the
original lhs had sufficient capacity. That kind of optimization will not be
achieved by any generic approach. There are places (and some would maintain
that there are quite a few such places) where the strong guarantee should
be abandoned in favor of a more efficient assignment. After all, if one
needs the strong guarantee in a certain place, one could always use

template < typename T >
T& safe_assign ( T& lhs, T rhs ) {
swap( lhs, rhs );
return ( lhs );
}

That way would be more in line with: you don't pay for what you don't use.


Best

Kai-Uwe Bux

 

[Marcus Kwok]

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.

All this talk of the assignment operator reminds me of these articles
talking about the issues with making an exception-safe assignment
operator:

http://icu.sourceforge.net/docs/papers/cpp_report/the_anatomy_of_the_assignment_operator.html

http://icu.sourceforge.net/docs/papers/cpp_report/the_assignment_operator_revisited.html

 

[LR]

Well, that would be

dummy & operator= ( dummy rhs ) {
swap( *this, rhs );
return ( *this );
}

which also calls a destructor and a copy constructor. Just the order is
different. On top of that, it calls swap, which usually is not very
expensive. The main advantage is that it provides the strong exception
safety guarantee.

I feel a little confused. Which version of swap are you calling?

LR

 

[Michiel.Salters]

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

This should very rarely be the case. It would violate the Assignable
property for STL container elements. I would expect that some sort
implementations can do self-assignments, so the STL has good
reasons.

When sorting ints, adding an extra int* comparision just in case
would be rather expensive. Just assume x = x is cheap. Any class
designer can add the obvious check if it really speeds things up.

HTH,
Michiel Salters

 

[Kai-Uwe Bux]

I feel a little confused. Which version of swap are you calling?

The copy-swap assignment idiom requires an overloaded swap method since the
default std::swap function may use the assignment operator, which we just
want to define.

The code above is written under the assumption that swap( dummy&, dummy& )
has been defined in the same namespace as dummy, in which case it should be
found. Had I done

dummy & operator= ( dummy rhs ) {
std::swap( *this, rhs );
return ( *this );
}

it would be necessary to overload std::swap for dummy (which is allowed, and
may be better practice -- I have not yet formed a firm opinion on that).


Best

Kai-Uwe Bux

 

[Kai-Uwe Bux]

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.

I think a reference counted non-intrusive smart pointer could benefit from
the first version. Consider the following schematic implementation:

template < typename T >
class refcount_ptr;

template < typename T >
void swap ( refcount_ptr< T > &, refcount_ptr< T > & );

template < typename T >
class refcount_ptr {

friend void swap<> ( refcount_ptr<T> &, refcount_ptr<T> & );

struct T_count {

T* t_ptr;
unsigned long ref_count;

T_count ( T * ptr )
: t_ptr( ptr )
, ref_count ( 1 )
{}

~T_count ( void ) {
delete ( t_ptr );
}

};

T_count * c_ptr;

public:

refcount_ptr ( T * ptr = 0 )
: c_ptr( new T_count ( ptr ) )
{}

refcount_ptr ( refcount_ptr const & other )
: c_ptr ( other.c_ptr )
{
++ c_ptr->ref_count;
}

~refcount_ptr ( void ) {
-- c_ptr->ref_count;
if ( c_ptr->ref_count == 0 ) {
delete( c_ptr );
}
}

refcount_ptr & operator= ( refcount_ptr const & other ) {
if ( c_ptr != other.c_ptr ) {
this->~refcount_ptr();
new ( this ) refcount_ptr( other );
}
return( *this );
}

T const * operator-> ( void ) const {
return( c_ptr->t_ptr );
}

T * operator-> ( void ) {
return( c_ptr->t_ptr );
}

// more stuff

};

In this case, the additional cost of the comparison could be zero: the
refcount_ptr objects fit within registers. It might very well be that
comparing the values of the c_ptr members is more efficient than comparing
their addresses. On the other hand, the value comparison will safe on
hidden self-assignments, which actually could be quite frequent depending
on the application.


Best

Kai-Ue Bux

 

[Frederick Gotham]

Noah Roberts posted:

Well, if it truely is not recoverable then #2 is true, but not
otherwise.

Regardless of whether a "Hand" can recover from being assigned to itself,
we can still prevent self-assignment in Debug Mode by using an "assert".

If you can recover then the check needs to happen anyway.

I don't know where you're getting that from.

Such is the case in self assignment and as you'll see when you go back
and read my original reply that is the primary reason an assert here is
invalid.

I still don't see why you think it's invalid.

If you say so. I think you should go do some more reading and testing
before making that assumption.

I don't need to do testing -- I know that there's a thing called "time",
the basic unit of which is the second. I also know that there's a thing
called "memory", the basic unit of which is the bit.

First: What third party? Are you bringing someone new into the
argument?

The "Third Party" is the person who wrote the reusable code.

Does it? By how much?

This figure varies from system to system, compiler to compiler, and is
influenced by numerous other things...

Anyone that has ever worked on a long term and/or large project in a
team understands that such assumptions are bad assumptions to make.

Not if we decide that an object of a particular type shouldn't be assigned
to itself.

Besides, we are talking assignment, not comparison.

I meant to write "assigned" rather than "compared". (I have correct the
quote above.)

But let's look at your example with a card hand. A card hard should
always be equal to itself no? Is there any condition in which it
wouldn't be? So why is it so perverse that you compare it to itself?
What are the concequences if it is?

Let's say that we're writing fast code. Let's say that, for some reason, it
takes a long time to compare one hand to another, so we want to speed up
the process as much as we can. Someone comes up with the idea of writing:

if(this==&rhs)return*this;

This will speed up considerably any self-comparison, but will slow down any
proper comparison between different objects. Let's go on to say that, if a
Hand is compared to itself (or perhaps assigned to itself) then that
comparison algorithm will invoke undefined behaviour. An "assert" would
work nicely to catch programming errors.

And there is no possibily for UB here.

The behaviour is undefined if you compare two pointers which don't point
into the same array. The following invokes UB:

int array1[5], array2[5], *p1 = array1+3, *p2 = array2+2;

p2 > p1; /* UB! */

 

[Noah Roberts]

Noah Roberts posted:



I don't know where you're getting that from.

Think about it. Your code was something like this:

Obj & Obj:perator = (Obj const & other)
{
assert (this != &other);

do copy stuff...

return *this;
}

Now, an assert will only catch the cases you test during debugging. It
will not catch anything that happens under release. Therefor, when you
are in release the check doesn't happen and the assigment op goes
straight into "do copy stuff". If this is a self assignment "do copy
stuff" probably explodes or reacts in some peculiar way. In order to
keep this from affecting the user (through a crash or worse, bad
results) the check has to be there anyway:

Obj & Obj:perator = (Obj const & other)
{
assert (this != &other);
if (this == &other) return *this;

do copy stuff...

return *this;
}

So your argument that using an assert is more efficient doesn't hold
water when you take care in the release build and acknowledge the FACT
that some bugs will get through no matter what precautions you take.

I still don't see why you think it's invalid.

It's pointless. It also causes numerous problems by breaking the
standard operation of operator =.

I don't need to do testing -- I know that there's a thing called "time",
the basic unit of which is the second. I also know that there's a thing
called "memory", the basic unit of which is the bit.

Statements of performance should always be backed up by real numbers.
What you are advocating is known as premature optimization. Not only
that but it is a *micro* optimization and under almost all situations
is utterly without merrit and won't have any affect on the speed of
execution or memory footprint.

The "Third Party" is the person who wrote the reusable code.

Your method of clipping makes it difficult to follow the conversation.
Is that on purpose?

The person who wrote the reusable code is the person under discussion
so it isn't a *third* person, they are the *first* person. Code like
that shouldn't be written.

This figure varies from system to system, compiler to compiler, and is
influenced by numerous other things...

As I've argued, there is no difference given considerations of real
world problems and the optimization you are trying to make will likely
never be worth the problems. When it is, operator = should be disabled
by making it private and the copy code should be in a function so as to
advertize that this isn't a normal copy operator.

Let's say that we're writing fast code. Let's say that, for some reason, it
takes a long time to compare one hand to another, so we want to speed up
the process as much as we can. Someone comes up with the idea of writing:

if(this==&rhs)return*this;

This will speed up considerably any self-comparison, but will slow down any
proper comparison between different objects. Let's go on to say that, if a
Hand is compared to itself (or perhaps assigned to itself) then that
comparison algorithm will invoke undefined behaviour. An "assert" would
work nicely to catch programming errors.

Are we talking about comparison or assignment??? You keep using one
where it appears you may need the other.

if (this == &other) comparison == equal!
if (this == &other) copy == noop.

Now, you are asserting that doing those tests will somehow impact the
performance of a given, highly theoretical program. Ok, let's assume
that you are right. For some program somewhere, a very unique program,
you need to optimize comparison and assignment so much that you cannot
allow the normal safety checks to make it into release code. First,
you damn well better have some real numbers to reach that conclusion.
Second, such a micro optimization (three or four machine code
operations at most) is the LAST thing to look at doing. Finally, you
would certainly want to advertize the problem more clearly by disabling
these operators and creating new ones so that nobody is confused about
the fact that these are not the operators they are expecting.

And there is no possibily for UB here.

The behaviour is undefined if you compare two pointers which don't point
into the same array. The following invokes UB:

int array1[5], array2[5], *p1 = array1+3, *p2 = array2+2;

p2 > p1; /* UB! */

I don't really even know what you are talking about here as your
clipping has completely removed context but I can see right off that
there is no undefined behavior in the above code. I can't imagine why
you think there is.