Re: Problem with copy constructor.
On May 1, 8:55 pm, pallav <pallavgu...@gmail.com> wrote:
I'm having some trouble with my copy constructor. I've tried using gdb
to find the bug, but it seg faults in the destructor. I'm not able to
see what I'm doing wrong. Since I'm using pointers, I need deep copy
and I believe I'm doing that in my constructors. Can someone help me
see what I'm missing out? Here is my code.
typedef boost::shared_ptr<Factor> FactorPtr;
enum FactorTypeT
{ FACTOR_ZERO, FACTOR_ONE, FACTOR_AND, FACTOR_OR};
class Factor
{
private:
struct FactorImpl *fact;
Factor();
Factor(const FactorTypeT type, const int index,
const Factor *next, const Factor *same);
Factor(const Factor& rhs);
static FactorPtr convNodeToFactor(const NodePtr& n, const NodePtr&
ntree);
static FactorPtr convNodeToFactorRecur(const NodePtr& n, const
NodePtr& ntree);
public:
~Factor();
static FactorPtr create();
static FactorPtr create(const FactorPtr& rhs);
};
struct FactorImpl
{
FactorTypeT type;
int idx;
struct FactorImpl *nextlevel;
struct FactorImpl *samelevel;
FactorImpl()
: type(FACTOR_UNKNOWN), idx(-1), nextlevel(0), samelevel(0) {}
// copy constructor
FactorImpl(const FactorImpl& rhs)
{
*this = rhs;
This line cannot work. You cannot, in general, implement copy
construction in terms of assignment, since copy construction
supposes that the left hand argument is fully constructed.
There are several ways around this, to avoid duplicating code;
in your case, just adding the initializer list from the default
constructor would work. However...
}
~FactorImpl()
{
if (nextlevel)
delete nextlevel;
if (samelevel)
delete samelevel; <------------ it segfaults here
You don't need the tests for NULL.
}
FactorImpl& operator=(const FactorImpl& rhs)
{
if (this != &rhs)
Red flag. If you have to test for self assignment, you're
usually doing something wrong...
{
type = rhs.type;
idx = rhs.idx;
if (rhs.nextlevel)
nextlevel = new FactorImpl(*rhs.nextlevel); <--- deep copy
right?
First, you're leaking memory. What happens to what nextlevel
pointed to before. You need a delete nextlevel here. But also,
what happens if there is an exception here or in the treatment
of samelevel? You end up with an object in a very strange
state.
The basic principle is right, but you're doing it in the wrong
place.
if (rhs.samelevel)
samelevel = new FactorImpl(*rhs.samelevel); <-- deep copy
right?
}
return *this;
}
This class seems to just scream for the swap idiom:
FactorImpl::FactorImpl(
FactorImpl const& other )
: type( other.type )
, idx( other.idx )
, nextlevel( other.nextlevel == NULL
? NULL
: new FactorImpl( *other.nextlevel ) )
, samelevel( other.samelevel == NULL
? NULL
: new FactorImpl( *other.samelevel ) )
{
}
FactorImpl&
FactorImpl::operator=(
FactorImpl const& other )
{
FactorImpl tmp( other ) ;
swap( tmp ) ;
return *this ;
}
void
FactorImpl::swap(
FactorImpl& other )
{
std::swap( type, other.type ) ;
std::swap( idx, other.idx ) ;
std::swap( nextlevel, other.nextlevel ) ;
std::swap( samelevel, other.samelevel ) ;
}
Except that I'm not really sure that you even want to support
assignment. If this is the standard compilation firewall idiom,
you don't need assignment in the implementation class, you use
the swap idiom to implement assignment in the outer class.
void dump(std::ostream& f)
{
static std::string str[7] = {"-0-", "-1-", "AND", "OR", "???"};
f << this << " " << str[type] << " " << idx << "\n";
if (nextlevel)
nextlevel->dump(f);
if (samelevel)
samelevel->dump(f);
}
};
Factor::Factor()
: fact(new FactorImpl()) {}
Factor::Factor(const FactorTypeT type, const int index,
const Factor *next, const Factor *same)
: fact(new FactorImpl())
{
fact->type = type;
fact->idx = index;
if (next)
fact->nextlevel = new FactorImpl(*next->fact); <----
making deep copy?
if (same)
fact->samelevel = new FactorImpl(*same->fact); <---- making
deep copy ?
}
Factor::~Factor()
{
delete fact;
}
Factor::Factor(const Factor& rhs)
: fact(new FactorImpl())
{
*fact = *(rhs.fact);
}
Why not simply:
Factor::Factor(
Factor const& other )
: fact( new FactorImpl( *other.fact ) )
{
}
IMHO, you're doing far too much with assignment. Get copy
construction right, without assignment, and then use the swap
idiom, or something similar, for assignment. (Factor is so
simple, even the swap idiom seems like overkill:
Factor&
Factor::operator=(
Factor const& other )
{
FactorImpl* tmp = new FactorImpl( *other.fact ) ;
delete fact ;
fact = tmp ;
return *this ;
}
FactorPtr Factor::create()
{
FactorPtr f(new Factor());
return f;
}
FactorPtr Factor::create(const FactorPtr& rhs)
{
FactorPtr f(new Factor(*rhs));
return f;
}
FactorPtr Factor::convNodeToFactor(const NodePtr& n, const NodePtr&
ntree)
Since you've not shown us anything concerning the nodes, I can't
really comment on the rest. But in general, for recursive
structures like this, the rule is to do the recursion in the
copy constructor (and the destructor, obviously), and for
assignments, use the copy constructors to create a new instance
of the recursive structure, then either delete the old instance,
and assign the new, or---if the new instance is in a fully
formed object with an adequate destructor, to play pointer games
(swap) so that the current instance ends up with the new, and
the temporary with the old; the destructor of the temporary
object will then take care of the deletes, when we leave the
operator=.
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34