Is self assignment test valid?

Hello.

Popular example of assignment operator looks like the one below:
-----------------------
T& T:perator=(T const& rhs) {
if (&rhs != this)
// operations
return *this;
}
-----------------------
Is this example valid and have well defined behavior? As far as I know
rhs can be bound to temporary, which in some implementations can be
stored in CPU registers etc. If it is true, than expression "&rhs"
would not make any sense. If it is valid, than could I ask for some
explainations?

Thanks
Sorry for my bad english and using Google Groups

--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

On Sep 1, 11:40 pm, gmr...@o2.pl wrote:
> ...in some implementations can be
> stored in CPU registers etc. If it is true, than expression "&rhs"
> would not make any sense.

Implementation can not effect expectation. Compilers are free to
implement the standard library any way they want, but it has to work
as expected. This is no different. You may expect to take the address
of any object.

As far as I know, compilers are smart enough to know when an argument
is not passed through the registers, and I've actually never seen it.
I've looked at the assembly GCC produced and always see arguments
being added to the stack, even of basic types like int. Although,
there is the register keyword, I haven't tested it much.


--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

gmrtmp@o2.pl wrote:

> -----------------------
> T& T:perator=(T const& rhs) {
> if (&rhs != this)
> // operations
> return *this;
> }
> -----------------------
> Is this example valid and have well defined behavior?

Yes.

> if (&rhs != this)

This tests if the address of rhs is not the same as the address of the
object you're assigning to.

> As far as I know rhs can be bound to temporary, which in some
> implementations can be stored in CPU registers etc. If it is true, than
> expression "&rhs" would not make any sense. If it is valid, than could I
> ask for some explainations?

Well, if rhs is bound to a temporary, it is a temporary reference to an
object. But it still references the same object.

--
Ruurd


[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

On 2 Wrz, 06:40, gmr...@o2.pl wrote:

> -----------------------
> T& T:perator=(T const& rhs) {
> if (&rhs != this)
> // operations
> return *this;}
>
> -----------------------
> Is this example valid and have well defined behavior?

Yes (provided that "operations" make sense), but it is debatable
whether it is a good idiom.

The argument goes that if the reliability of the assignment operator
depends on the existence of the self-assignment-test, then the
operator most likely has some deeper design problems.
It is impossible to assert this for the code above, since the crucial
part ("operations") is not shown, but in practice this is very often
true - if you need this test, then you just try to protect something
that is inherently broken and even this protection is usually not
enough (hint: exception safety).

Performance optimization is the only potentially valid motivation for
such a test, but it actually improves the performance of a use case
that never happens (why assign to same object?) and therefore it does
not make any sense to optimize it.

> As far as I know
> rhs can be bound to temporary, which in some implementations can be
> stored in CPU registers etc.

It does not matter. You can take the address of every named object
(rhs is such an object) and the compiler will do the right thing.

--
Maciej Sobczak * www.msobczak.com * www.inspirel.com


[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Maciej Sobczak wrote:
> On 2 Wrz, 06:40, gmr...@o2.pl wrote:
>> -----------------------
>> Is this example valid and have well defined behavior?
>
> Yes (provided that "operations" make sense), but it is debatable
> whether it is a good idiom.
>
> The argument goes that if the reliability of the assignment operator
> depends on the existence of the self-assignment-test, then the
> operator most likely has some deeper design problems.
> It is impossible to assert this for the code above, since the crucial
> part ("operations") is not shown, but in practice this is very often
> true - if you need this test, then you just try to protect something
> that is inherently broken and even this protection is usually not
> enough (hint: exception safety).
>
Examples would be nice.

I remember reading before that self assignemnt protection very often
hides some flaws, but I really fail to see the "very often".
For any object where it makes sense to use e.g. a memcpy in the
copy-ctor the self assignment test would tend to make the copying code
simpler and not only faster, no?

> Performance optimization is the only potentially valid motivation for
> such a test, but it actually improves the performance of a use case
> that never happens (why assign to same object?) and therefore it does
> not make any sense to optimize it.
>
Now that's quite a strong assumtion, is it not? "Never happens" is just
something that's bound to happen sooner or later :-)

Anyway - we (I) build redundancy into my code so that even with the
inevitable bugs it may just continue to run.

br,
Martin

--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Martin T wrote:
> Maciej Sobczak wrote:
>>
>> The argument goes that if the reliability of the assignment operator
>> depends on the existence of the self-assignment-test, then the
>> operator most likely has some deeper design problems.
>> It is impossible to assert this for the code above, since the crucial
>> part ("operations") is not shown, but in practice this is very often
>> true - if you need this test, then you just try to protect something
>> that is inherently broken and even this protection is usually not
>> enough (hint: exception safety).
>>
> Examples would be nice.
>
> I remember reading before that self assignemnt protection very often
> hides some flaws, but I really fail to see the "very often".

I think this pattern happens often enough:

MyClass& MyClass:perator=(const MyClass& other)
{
if (&other != this) {
// destroy what *this has
// construct in *this a copy of what other has
}
return *this;
}

Guess what happens if the "copy" part throws an exception.

> For any object where it makes sense to use e.g. a memcpy in the
> copy-ctor the self assignment test would tend to make the copying code
> simpler and not only faster, no?

No simpler because of the additional test. Faster only in the rare
case of self-assignment; the additional test actually gives you a
penalty in most cases where the addresses are different.

Moreover, if a simple memcpy is enough, you don't need to define the
copy constructor or the assignment operator, anyway.

>
>> Performance optimization is the only potentially valid motivation for
>> such a test, but it actually improves the performance of a use case
>> that never happens (why assign to same object?) and therefore it does
>> not make any sense to optimize it.
>>
> Now that's quite a strong assumtion, is it not? "Never happens" is just
> something that's bound to happen sooner or later :-)

Not that we don't have to defend against self-assignment; just that
there's a better way that works in all cases and doesn't need the check
that gives you the penalty in most cases.

The standard idiom for the previous example goes like the following:

MyClass& MyClass:perator=(const MyClass& other)
{
// construct in local variables a copy of what other has
// swap *this and the local variables
}

--
Seungbeom Kim

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Martin T wrote:
> Maciej Sobczak wrote:
> [checking for self-assignment is not sufficient]
> Examples would be nice.

True.

> I remember reading before that self assignemnt protection very often
> hides some flaws, but I really fail to see the "very often".

Simple thing:

struct foo
{
string bar;
string baz;
};

If you assign to this structure, it could happen that the first string is
assigned and during the assignment of the second one a memory shortage
causes a bad_alloc exception. This leads to you ending up with a
half-assigned struct, which will usually mean an inconsistent state.
Replace 'string' and 'memory shortage' above with 'resource' and 'resource
shortage' to make it more general.

I guess the alternative that Maciej was thinking about (but never actually
mentioned) was the copy and swap approach:

operator=( T const& other)
{
T tmp(other); // make a copy
swap( *this, tmp);
return *this;
}

This first allocates necessary resources by making a copy and then swapping
with the target. However, this only works when swap() is implemented in a
way that it will never throw. Unfortunately, std::swap() isn't, because it
simply makes a copy and then uses assignment:

swap( T& t1, T& t2)
{
T tmp = t1;
t1 = t2;
t2 = tmp;
}

However, I think you can safely use swap on types of the standard library
(due to specialisations), or at least use the swap() memberfunction of most
types, so you can build your own swap based on it:

swap( foo& f1, foo& f2)
{
f1.bar.swap(f2.bar);
f1.baz.swap(f2.baz);
}


> For any object where it makes sense to use e.g. a memcpy in the
> copy-ctor the self assignment test would tend to make the copying code
> simpler and not only faster, no?

Well, how much faster? Did you profile it? Actually, I would check for
self-assignment out of habit, but using the copy and swap idiom is more
important, because it gives you some guarantees that you can't achieve
otherwise.

>> Performance optimization is the only potentially valid motivation for
>> such a test, but it actually improves the performance of a use case
>> that never happens (why assign to same object?) and therefore it does
>> not make any sense to optimize it.
>>
> Now that's quite a strong assumtion, is it not? "Never happens" is just
> something that's bound to happen sooner or later :-)

How often do you assign an object to itself? Is that worth optimising it for
speed? I'd say no. In any case though, an there the reason is the 'sooner
or later', I would always try to make it work correctly.

> Anyway - we (I) build redundancy into my code so that even with the
> inevitable bugs it may just continue to run.

It might continue to run, but with incorrect results, if the assignment is
half-way done.

Uli


--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Ulrich Eckhardt wrote:
> Martin T wrote:
>> Maciej Sobczak wrote:
>> [checking for self-assignment is not sufficient]
>> ...
>
> I guess the alternative that Maciej was thinking about (but never actually
> mentioned) was the copy and swap approach:
>
> operator=( T const& other)
> {
> T tmp(other); // make a copy
> swap( *this, tmp);
> return *this;
> }
>
> This first allocates necessary resources by making a copy and then swapping
> with the target.

Yes, I know the swap approach but have never used it so far.

I guess there are two ways to think of assignment/copy construction and
that's what got me a little bit confused in the first place.

a) - write a copy-ctor
- implement the operator= in terms of the copy-ctor (copy construct
temp + swap)
Copyable& operator=(Copyable const& o) {
Copyable tmp(o);
this->swap(tmp);
return *this;
}

b) - write operator=
- implement the copy-ctor in terms of operator= (default initialize
this and then assign)
Copyable2& operator=(Copyable2 const& o)
{
if(this == &o)
return *this;
clear();
p_ = new int[o.s_];
s_ = o.s_;
memcpy(p_, o.p_, s_*sizeof(int));
return *this;
}


If you use b), then a test for self assignment makes sense.
I think (a) makes MUCH more sense, but I have also seen (b) quite a few
times.

So I would like to say that if the code "needs" a check for
self-assignment, the the copy operator and copy constructor have been
written "the wrong way round" ...
.... and, well, yes - the code is kind of broken if an exception occurs :-)

cheers,
Martin

EXAMPLE CODE:
class Copyable {
public:
Copyable()
: p_(NULL),
s_(0)
{ }

explicit Copyable(size_t s)
: p_(NULL),
s_(0)
{
p_ = new int[s];
s_ = s;
}

~Copyable()
{
delete p_;
}

void swap(Copyable & o)
{
int* p = o.p_;
size_t s = o.s_;
o.p_ = p_;
o.s_ = s_;
p_ = p;
s_ = s;
}

Copyable(Copyable const& o)
{
p_ = new int[o.s_];
s_ = o.s_;
memcpy(p_, o.p_, s_*sizeof(int));
}

Copyable& operator=(Copyable const& o)
{
Copyable tmp(o);
this->swap(tmp);
return *this;
}
private:
int* p_;
size_t s_;
};

class Copyable2 {
public:
Copyable2()
: p_(NULL),
s_(0)
{ }

explicit Copyable2(size_t s)
: p_(NULL),
s_(0)
{
p_ = new int[s];
s_ = s;
}

~Copyable2()
{
delete p_;
}

void clear()
{
delete p_;
p_ = NULL;
s_ = 0;
}

void swap(Copyable2 & o)
{
int* p = o.p_;
size_t s = o.s_;
o.p_ = p_;
o.s_ = s_;
p_ = p;
s_ = s;
}

Copyable2(Copyable2 const& o)
: p_(NULL),
s_(0)
{
*this = o;
}

Copyable2& operator=(Copyable2 const& o)
{
if(this == &o)
return *this;
clear();
p_ = new int[o.s_];
s_ = o.s_;
memcpy(p_, o.p_, s_*sizeof(int));
return *this;
}

private:
int* p_;
size_t s_;
};

--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

On Sep 4, 12:29 pm, Ulrich Eckhardt <dooms...@knuut.de> wrote:
> How often do you assign an object to itself? Is that worth optimising it for
> speed? I'd say no. In any case though, an there the reason is the 'sooner
> or later', I would always try to make it work correctly.
>
> > Anyway - we (I) build redundancy into my code so that even with the
> > inevitable bugs it may just continue to run.
>
> It might continue to run, but with incorrect results, if the assignment is
> half-way done.

Still, checking for assingment-to-self as *optimization* isn't a very
good one. It gives penalty to ALL cases and benefit only to a FEW
(rare) cases. Statistically, bad choise.

You can write assignment-to-self correctly w/o knowing this. Example;

struct string
{
char* s;
int length;
};

Wrong:

.... operator = (const string& x)
delete[] s;
s = new char[x.length];
....

Right:

char* toDelete = s;
s = new char[x.length];
....
delete[] toDelete;

Problem: creating a temporary object, usually 1 extra ALU instruction.
But better than branch misprediction.



--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

on Thu Sep 04 2008, Ulrich Eckhardt <doomster-AT-knuut.de> wrote:

> I guess the alternative that Maciej was thinking about (but never actually
> mentioned) was the copy and swap approach:
>
> operator=( T const& other)
> {
> T tmp(other); // make a copy
> swap( *this, tmp);
> return *this;
> }
>

That's the wrong way to write it, though ;-). On real compilers

operator=( T other )
{
swap( *this, other);
return *this;
}

can be *much* more efficient because of copy elision.

Cheers,

--
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]