Problem with copy constructor.

[pallav]

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<FactorFactorPtr;

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;
}

~FactorImpl()
{
if (nextlevel)
delete nextlevel;
if (samelevel)
delete samelevel; <------------ it segfaults here
}

FactorImpl& operator=(const FactorImpl& rhs)
{
if (this != &rhs)
{
type = rhs.type;
idx = rhs.idx;
if (rhs.nextlevel)
nextlevel = new FactorImpl(*rhs.nextlevel); <--- deep copy
right?
if (rhs.samelevel)
samelevel = new FactorImpl(*rhs.samelevel); <-- deep copy
right?
}
return *this;
}

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);
}

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)
{
switch (ntree->getFunction())
{
case NODE_ZERO:
return FactorPtr(new Factor(FACTOR_ZERO, -1, 0, 0));
case NODE_ONE:
return FactorPtr(new Factor(FACTOR_ONE, -1, 0, 0));
default:
return Factor::convNodeToFactorRecur(n, ntree);
}
}

FactorPtr Factor::convNodeToFactorRecur(const NodePtr& n, const
NodePtr& ntree)
{
if (ntree->getType() != NODE_UNASSIGNED)
return FactorPtr(new Factor(FACTOR_LEAF, n->faninIndex(ntree), 0,
0));

unsigned int j;
NodePtr fanin;
FactorPtr lit, tmp, gand, gor, curand, curor;

gor = curor = FactorPtr();
for (int i = ntree->numCubes() - 1; i >= 0; --i)
{
gand = curand = FactorPtr();
foreach_fanin(ntree, j, fanin)
{
switch (ntree->getLiteral(i, j))
{
case VZERO:
tmp = Factor::convNodeToFactorRecur(n, fanin);
lit = FactorPtr(new Factor(FACTOR_INV, -1, 0, tmp.get()));
<------- this line causes a segfault. the problem is with pass
tmp.get() (which is a valid pointer to object) so I need to do a deep
copy. not sure if i'm doing deep copy correctly.
break;
case VONE:
lit = Factor::convNodeToFactorRecur(n, fanin);
break;
default:
break;
}
if (!curand)
gand = curand = lit;
else
{
curand->fact->samelevel = (lit.get())->fact;
curand = lit;
}
}

if (!gand)
bailOut("Factor::convNodeToFactorRecur()",
ERROR_NODE_NOT_MINIMALBASE_FUNCTION);
else if (!gand->fact->samelevel)
gand = FactorPtr(new Factor(FACTOR_AND, -1, 0, gand.get()));

if (!gor)
gor = curor = gand;
else
{
curor->fact->samelevel = (gand.get())->fact;
curor = gand;
}
}

if (!gor)
bailOut("Factor::convNodeToFactorRecur()",
ERROR_NODE_NOT_MINIMALBASE_FUNCTION);
else if (!gor->fact->samelevel)
gor = FactorPtr(new Factor(FACTOR_OR, -1, 0, gor.get()));

return gor;
}

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xbf7ffffc
0x000b3b20 in FactorImpl::~FactorImpl (this=0xd010c0) at factor.cpp:52
52 delete samelevel;

thanks for any help.

[Obnoxious User]

pallav skrev:

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.

[snip]

It would be easier if the code was compilable/runnable.

--
OU

[Victor Bazarov]

pallav 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<FactorFactorPtr;

enum FactorTypeT
{ FACTOR_ZERO, FACTOR_ONE, FACTOR_AND, FACTOR_OR};

class Factor
{
private:
struct FactorImpl *fact;

Factor();
Factor(const FactorTypeT type, const int index,

Top-level const's are meaningless.

const Factor *next, const Factor *same);
Factor(const Factor& rhs);

static FactorPtr convNodeToFactor(const NodePtr& n, const NodePtr&

What's "NodePtr"?

ntree);
static FactorPtr convNodeToFactorRecur(const NodePtr& n, const
NodePtr& ntree);

public:
~Factor();
static FactorPtr create();
static FactorPtr create(const FactorPtr& rhs);
};

The "Rule of Three" is violated here.

[..]

Also, there is no need to verify that the pointer is non-null before
using 'delete' on it.

Also, you have memory leaks -- in the FactorImpl::operator= you don't
deallocate (delete) the pointers.

Also, using the assignment operator in the copy-constructor is counter-
intuitive. Using them the other way around might be OK...

See FAQ 5.8.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[pallav]

On May 1, 2:55 pm, pallav <[e-mail]:

I'm having some trouble with my copy constructor. I've tried using gdb

regarding the post above, i did some more investigating and have
narrowed the problem down to these two functions:

FactorImpl(const FactorImpl& rhs)
{
*this = rhs;
}

FactorImpl& operator=(const FactorImpl& rhs)
{
if (this != &rhs)
{
type = rhs.type;
idx = rhs.idx;
if (rhs.nextlevel)
nextlevel = new FactorImpl(*rhs.nextlevel);
if (rhs.samelevel)
samelevel = new FactorImpl(*rhs.samelevel);
}
return *this;
}

when the operator= is entered *this contains the following values

type = 13691
idx = 0
nextlevel = 0x0
samelevel = 0x3

so its an initalization problem. since rhs.samelevel = 0x0 samelevel
is never modified from the garbage values and hence the segfault.

if i understand correctly *this = rhs means call the constructor on
this and then do the =. correct? this means that FactorImpl() should
have been called on this and *this should be equal to

type = 0
idx = -1
nextlevel = 0x0
samelevel = 0x0

then the assignment would take place. is this not correct? i have to
copy the variable initializer list in operator=?

thanks

[Victor Bazarov]

pallav wrote:

On May 1, 2:55 pm, pallav <[e-mail]:

>I'm having some trouble with my copy constructor. I've tried using
gdb

regarding the post above, i did some more investigating and have
narrowed the problem down to these two functions:

FactorImpl(const FactorImpl& rhs)
{

Right here all the members in 'this' object contain garbage since you
don't initialise them.

*this = rhs;
}

FactorImpl& operator=(const FactorImpl& rhs)
{
if (this != &rhs)
{
type = rhs.type;
idx = rhs.idx;
if (rhs.nextlevel)
nextlevel = new FactorImpl(*rhs.nextlevel);
if (rhs.samelevel)
samelevel = new FactorImpl(*rhs.samelevel);
}
return *this;
}

when the operator= is entered *this contains the following values

type = 13691
idx = 0
nextlevel = 0x0
samelevel = 0x3

so its an initalization problem. since rhs.samelevel = 0x0 samelevel
is never modified from the garbage values and hence the segfault.

Yep.

if i understand correctly *this = rhs means call the constructor on
this

Huh?

and then do the =. correct? this means that FactorImpl() should
have been called on this and *this should be equal to

type = 0
idx = -1
nextlevel = 0x0
samelevel = 0x0

Huh?

>
then the assignment would take place. is this not correct? i have to
copy the variable initializer list in operator=?

Yes, I think that's the answer.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[pallav]

Hi,

Thanks for your time/help on this.

Top-level const's are meaningless.

const Factor *next, const Factor *same);

I'm not sure what you mean by this. You're saying it is not necessary
to make the arguments to constructors as const?

What's "NodePtr"?

It is typedef boost::shared_ptr<class NodeNodePtr;

The "Rule of Three" is violated here.

Here is what I have now. Is this correct way?

struct FactorImpl
{
FactorTypeT type;
int idx;
struct FactorImpl *nextlevel;
struct FactorImpl *samelevel;

FactorImpl()
: type(FACTOR_UNKNOWN), idx(-1), nextlevel(0), samelevel(0) {}

// copy
FactorImpl(const FactorImpl& rhs)
: type(rhs.type), idx(rhs.idx), nextlevel(0), samelevel(0)
{
if (rhs.nextlevel)
nextlevel = new FactorImpl(*rhs.nextlevel);
if (rhs.samelevel)
samelevel = new FactorImpl(*rhs.samelevel);
}

// destructor
~FactorImpl()
{
delete nextlevel;
delete samelevel;
}

// helper for assignment
void swap(FactorImpl& rhs)
{
std::swap(type, rhs.type);
std::swap(idx, rhs.idx);
std::swap(nextlevel, rhs.nextlevel);
std::swap(samelevel, rhs.samelevel);
}

// assignment
FactorImpl& operator=(const FactorImpl& rhs)
{
FactorImpl tmp(rhs);
tmp.swap(*this);
return *this;
}
};

class Factor
{
private:
struct FactorImpl *fact;

Factor();
Factor(const FactorTypeT type, const int index,
const Factor *next, const Factor *same);
Factor(const Factor& rhs);
Factor& operator=(const Factor& rhs);
void swap(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);
void dump(std::ostream& f);

// I have a private ctor/assignment operator. I don't want the user
to be able to do assignment outside
// the class. Also I have done typedef boost::shared_ptr<Factor>
FactorPtr so the object is returned via a boost_shared ptr since it
will get attached to a node.
};

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);
if (same)
fact->samelevel = new FactorImpl(*same->fact);
}

Factor::~Factor()
{
delete fact;
}

// copy constructor
Factor::Factor(const Factor& rhs)
: fact(new FactorImpl(*rhs.fact)) {}

// helper for assignment
void Factor::swap(Factor& rhs)
{
std::swap(fact, rhs.fact);
}

// assignment operator
Factor& Factor::operator=(const Factor& rhs)
{
Factor tmp(rhs);
tmp.swap(*this);
return *this;
}

// public static create functions
FactorPtr Factor::create()
{
FactorPtr f(new Factor());
return f;
}

FactorPtr Factor::create(const FactorPtr& rhs)
{
FactorPtr f(new Factor(*rhs));
return f;
}

Also, there is no need to verify that the pointer is non-null before
using 'delete' on it.

Thanks for that.

Also, you have memory leaks -- in the FactorImpl::operator= you don't
deallocate (delete) the pointers.

Yes I see it now.

Also, using the assignment operator in the copy-constructor is counter-
intuitive. Using them the other way around might be OK...

I think i'll use the swap/copy constructor to implement the assignment
operator. The above code seems to work. Thanks for your time and help
again.

[Victor Bazarov]

pallav wrote:

Thanks for your time/help on this.

Anytime.

>Top-level const's are meaningless.

>> const Factor *next, const Factor *same);

I'm not sure what you mean by this. You're saying it is not necessary
to make the arguments to constructors as const?

The ones you quoted here aren't the ones I referred to. These aren't
top-level. These are:

int foo(const int blah, const double blahblah);

>

>What's "NodePtr"?

It is typedef boost::shared_ptr<class NodeNodePtr;

You didn't mention it before. It was prohibiting us from compiling
your code. See FAQ 5.8.

>

>The "Rule of Three" is violated here.

Here is what I have now. Is this correct way?

I think I wrote that about Factor, not FactorImpl.

[..]

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[James Kanze]

On May 1, 8:55 pm, pallav <[e-mail]:

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<FactorFactorPtr;

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:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]