Problem with copy constructor.

P

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

~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::eek:stream& 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.
 
O

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.
Click to expand...
[snip]

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

Victor Bazarov

pallav said:
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,
Click to expand...

Top-level const's are meaningless.
const Factor *next, const Factor *same);
Factor(const Factor& rhs);

static FactorPtr convNodeToFactor(const NodePtr& n, const NodePtr&
Click to expand...

What's "NodePtr"?
ntree);
static FactorPtr convNodeToFactorRecur(const NodePtr& n, const
NodePtr& ntree);

public:
~Factor();
static FactorPtr create();
static FactorPtr create(const FactorPtr& rhs);
};
Click to expand...

The "Rule of Three" is violated here.
[..]
Click to expand...

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::eek:perator= 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
 
P

pallav

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

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
 
V

Victor Bazarov

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

FactorImpl(const FactorImpl& rhs)
{
Click to expand...

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=?
Click to expand...

Yes, I think that's the answer.

V
 
P

pallav

Hi,

Thanks for your time/help on this.
Top-level const's are meaningless.
Click to expand...

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"?
Click to expand...

It is typedef boost::shared_ptr said:
The "Rule of Three" is violated here.
Click to expand...

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::eek:stream& 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::eek:perator=(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.
Click to expand...

Thanks for that.
Also, you have memory leaks -- in the FactorImpl::eek:perator= you don't
deallocate (delete) the pointers.
Click to expand...

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...
Click to expand...

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

Victor Bazarov

pallav said:
Thanks for your time/help on this.
Anytime.


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

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);
It is typedef boost::shared_ptr<class Node> NodePtr;
Click to expand...

You didn't mention it before. It was prohibiting us from compiling
your code. See FAQ 5.8.
Here is what I have now. Is this correct way?
Click to expand...

I think I wrote that about Factor, not FactorImpl.
[..]
Click to expand...

V
 
J

James Kanze

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.
Click to expand...
typedef boost::shared_ptr<Factor> FactorPtr;
Click to expand...
enum FactorTypeT
{ FACTOR_ZERO, FACTOR_ONE, FACTOR_AND, FACTOR_OR};
Click to expand...
class Factor
{
private:
struct FactorImpl *fact;
Click to expand...
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);
Click to expand...
public:
~Factor();
static FactorPtr create();
static FactorPtr create(const FactorPtr& rhs);
};
Click to expand...
struct FactorImpl
{
FactorTypeT type;
int idx;
struct FactorImpl *nextlevel;
struct FactorImpl *samelevel;
Click to expand...
FactorImpl()
: type(FACTOR_UNKNOWN), idx(-1), nextlevel(0), samelevel(0) {}

// copy constructor
FactorImpl(const FactorImpl& rhs)
{
*this = rhs;
Click to expand...

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...
}
Click to expand...
~FactorImpl()
{
if (nextlevel)
delete nextlevel;
if (samelevel)
delete samelevel; <------------ it segfaults here
Click to expand...

You don't need the tests for NULL.
}

FactorImpl& operator=(const FactorImpl& rhs)
{
if (this != &rhs)
Click to expand...

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?
Click to expand...

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;
}
Click to expand...

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::eek:perator=(
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::eek:stream& 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);
}

};
Click to expand...
Factor::Factor()
: fact(new FactorImpl()) {}
Click to expand...
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 ?
}
Click to expand...
Factor::~Factor()
{
delete fact;
}
Click to expand...
Factor::Factor(const Factor& rhs)
: fact(new FactorImpl())
{
*fact = *(rhs.fact);

}
Click to expand...

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::eek:perator=(
Factor const& other )
{
FactorImpl* tmp = new FactorImpl( *other.fact ) ;
delete fact ;
fact = tmp ;
return *this ;
}
FactorPtr Factor::create()
{
FactorPtr f(new Factor());
return f;
}
Click to expand...
FactorPtr Factor::create(const FactorPtr& rhs)
{
FactorPtr f(new Factor(*rhs));
return f;
}
Click to expand...
FactorPtr Factor::convNodeToFactor(const NodePtr& n, const NodePtr&
ntree)
Click to expand...

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

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Why the destructor executes after its friend operator (in which thereis a copy constructor) runs 5
Can I declare a variable with an uncertainty number suffix? 3
Copy constructor problem 11
private copy constructor question 9
Issue with passing fetched data to POST form. How can I? 0
Copy Constructor and const 13
Copy Constructor with Template Class 2
Logic Problem with BigInteger Method 2

Members online

No members online now.
Total: 30 (members: 0, guests: 30)
Robots: 390

Forum statistics

Threads
473,770
Messages
2,569,584
Members
45,075
Latest member
MakersCBDBloodSupport

Latest Threads

Top