It looks like bad programming, but is it?

[John Fullman]

I do this in several large classes to avoid rewriting copy ctr code. It
seems to work okay, but I was wondering if I might be shooting myself
in the foot later... I can't think of anything wrong with it, but it
looks like bad programming.

class MyClass
{
public:
//...

//Assignment operator defined
MyClass& operator=(const MyClass& copy)
{
if(this != &copy)
{
//... do the copy...
}
}

//Copy constructor
MyClass(const MyClass& copy)
{
*(const_cast<MyClass*>(this)) = copy;
}

//...
};

 

[Andre Kostur]

I do this in several large classes to avoid rewriting copy ctr code. It
seems to work okay, but I was wondering if I might be shooting myself
in the foot later... I can't think of anything wrong with it, but it
looks like bad programming.

class MyClass
{
public:
//...

//Assignment operator defined
MyClass& operator=(const MyClass& copy)
{
if(this != &copy)
{
//... do the copy...
}
}

//Copy constructor
MyClass(const MyClass& copy)
{
*(const_cast<MyClass*>(this)) = copy;
}

//...
};

Depends on your class. But as a rule of thumb, probably not the
greatest:

1) That copy constructor's gonna do _bad_ things when you start to
inherit stuff. (It dices! It slices!.....)

2) You pay the cost of default constructing your class (and all member
variables, and parent classes), then throw away all of that work by re-
assigning most (all?) of the values almost immediately.

3a) How often do you actually perform a self-assignment? Why pay the
cost of the comparision if you never actually self-assign.

3b) Is your assignment code actually that sensitive to self-assignment?
If your assignment code does the right thing, even for self-assignment,
then you don't need to do the compare and just self-assign. (Also
consider exception safety concerns...)

4) That const_cast is an immediate red flag that something's wrong.... (I
didn't think this was const in a copy constructor... why is that
const_cast there at all?)

 

[red floyd]

I do this in several large classes to avoid rewriting copy ctr code. It
seems to work okay, but I was wondering if I might be shooting myself
in the foot later... I can't think of anything wrong with it, but it
looks like bad programming.

class MyClass
{
public:
//...

//Assignment operator defined
MyClass& operator=(const MyClass& copy)
{
if(this != &copy)
{
//... do the copy...
}
}

//Copy constructor
MyClass(const MyClass& copy)
{
*(const_cast<MyClass*>(this)) = copy;
}

//...
};

How about:

class MyClass
{
private:
CopyFrom(const MyClass& src);
public:
MyClass& operator=(const MyClass& copy)
{
if (this != &copy)
{
CopyFrom(copy);
}
}

MyClass(const MyClass& copy)
{
CopyFrom(copy);
}

 

[John Harrison]

I do this in several large classes to avoid rewriting copy ctr code. It
seems to work okay, but I was wondering if I might be shooting myself
in the foot later... I can't think of anything wrong with it, but it
looks like bad programming.

The recommended way is to write the assignment operator in terms of the
copy constructor, not the other way around.

class X
{
public:
X(const X& rhs);
void swap(X& rhs);
X& operator=(const X& rhs)
{
X tmp(rhs);
swap(tmp);
return *this;
}
};

This has the advantage of being exception safe, as well as avoiding code
duplication. Swap is usually very easy to write (and useful in it's own
right).

But the best way is as far possible to write classes where the default
copy constructor and assignment operator do the right thing, e.g. avoid
using pointers unless you have very good reason.

john

 

[John Fullman]

Depends on your class. But as a rule of thumb, probably not the
greatest:

1) That copy constructor's gonna do _bad_ things when you start to
inherit stuff. (It dices! It slices!.....)
2) You pay the cost of default constructing your class (and all member
variables, and parent classes), then throw away all of that work by re-
assigning most (all?) of the values almost immediately.

This is a valid point. I think this is where the down side really comes
into play. So, as long as I avoid inheritance.. lol ... I'm probably
okay.

3a) How often do you actually perform a self-assignment? Why pay the
cost of the comparision if you never actually self-assign.

3b) Is your assignment code actually that sensitive to self-assignment?
If your assignment code does the right thing, even for self-assignment,
then you don't need to do the compare and just self-assign. (Also
consider exception safety concerns...)

My reasons for protecting against self assignment is 3 fold:

1. one compare is less intense than actually performing the self
assignment (espeicially being that this class is very large on the
stack on on the heap) It's a small price to pay.

2. In my code, I am passing pointers and references to these class
around like I'm in a 3 ring circus (also because the class is expensive
to copy) The class also contains pointers to other instances of the
same time. This is the makings for a scenario when self assignment is
likley.

3. My professor said I should

4) That const_cast is an immediate red flag that something's wrong.... (I
didn't think this was const in a copy constructor... why is that
const_cast there at all?)

Yea. The const_cast is why I posted it. I kinda figured I might be
doing something potentially unstable. I think your right on with the
inheritance issue. At least, at this time, it's a stand-alone class.

Appreciate the input.

 

[David White]

Yea. The const_cast is why I posted it. I kinda figured I might be
doing something potentially unstable.

There's nothing unstable about it. It's just completely unnecessary to cast
'this' into something that it already is. The cast is effectively a "no
operation", but it does succeeed in making your code look cluttered, messy
and confusing. All you needed was:
*this = copy;

DW

 

[msalters]

John Fullman schreef:

My reasons for protecting against self assignment is 3 fold:

1. one compare is less intense than actually performing the self
assignment (espeicially being that this class is very large on the
stack on on the heap) It's a small price to pay.

Have you measured it? The additional small price is always spent,
whereas the potentially big copy is very rare. Fast*Frequent might
be slower than Slow*Rare. In fact, in many programs self assignment
never happens, so the fast check is still a slowdown.

2. In my code, I am passing pointers and references to these class
around like I'm in a 3 ring circus (also because the class is expensive
to copy) The class also contains pointers to other instances of the
same time. This is the makings for a scenario when self assignment is
likley.
Yep.

3. My professor said I should

He should know better. I hope he has read Herb Sutter's work.

Michiel Salters

 

[Karl Heinz Buchegger]

John Fullman schreef:


Have you measured it? The additional small price is always spent,
whereas the potentially big copy is very rare. Fast*Frequent might
be slower than Slow*Rare. In fact, in many programs self assignment
never happens, so the fast check is still a slowdown.

Actually I once measured that with some vector algebra classes that were
used a lot in our programs.
For our matrix it turned out that in roughly 35 million assignments (then
I stopped checking) there wasn't a single self assignments. Thus I would
have to pay the price for at least 35 million checks just to potentially avoid
assigning 12 double values.

 

[John Fullman]

There's nothing unstable about it. It's just completely unnecessary to cast
'this' into something that it already is. The cast is effectively a "no
operation", but it does succeeed in making your code look cluttered, messy
and confusing. All you needed was:
*this = copy;

DW

Wow! You're right. I think it's because the this pointer is a:
MyClass const *
I guess it makes sense that I don't need it. Learn something new every
day

 

[Protoman]

What's the swap funct do? How do you write it?

 

[Protoman]

What's the swap funct do? How do you write it?

 

[Mark P]

What's the swap funct do? How do you write it?

Please don't post the same message twice.

The swap function swaps the values of two objects. Here's a simple example:

#include <iostream>
using namespace std;

class A
{
public:
A(int i_) : i(i_) {}
void swap(A& rhs);
int getVal() const {return i;}
private:
int i;
};

void A::swap (A& rhs)
{
int temp = i;
i = rhs.i;
rhs.i = temp;
}

int main()
{
A x(1);
A y(2);
cout << x.getVal() << " " << y.getVal() << endl;
x.swap(y);
cout << x.getVal() << " " << y.getVal() << endl;
}

The output is then:
1 2
2 1

Mark

 

[Protoman]

Got it.