again the problem: the destructor is called twice

[David]

Hi all,

I posted my question two days ago, and tried to solve this problem.
but until now I didn't solve that. and I cut my codes so maybe this
time it is more readable.

/////////////////////////////////////////////////////
#ifndef MYCLASS_H_
#define MYCLASS_H_

#include <string>
#include <list>
//#include "name.h"

using namespace std;
class myclass
{
protected:
list<string> namelist;
// map<int,vector<name> >names;
public:

myclass();
myclass(const myclass &my);
~myclass();
// myclass& operator=(const myclass &it);
void AddName(const string &name);
void GetMyclass();
};

#endif

///////////////////////////////////////////////
myclass.cpp
#include "myclass.h"
#include <algorithm>
#include <iostream>

myclass::myclass()
{

}

myclass::myclass(const myclass &my)
{

namelist=my.namelist;
}

myclass::~myclass()
{

namelist.erase(namelist.begin(),namelist.end());
}


void myclass::AddName(const string &name)
{
list<string>::iterator ii;
ii=find(namelist.begin(),namelist.end(),name);

if(ii==namelist.end())
namelist.push_back(name);
}

void myclass::GetMyclass()
{
list<string>::iterator ii;
for(ii=namelist.begin();ii!=namelist.end();ii++)
cout<<*ii<<endl;
}

/////////////////////////////////////////////////
test.h
#ifndef TEST_H_
#define TEST_H_

#include <string>
#include <map>
#include "myclass.h"

using namespace std;

class test
{
protected:
map<string,myclass*> tests;

public:
test();
test(const test& mytest);
test& operator=(const test& ts);
~test();
void AddMyclass(const string &name,const myclass &my);
void GetTest();

};

#endif

////////////////////////////////////////////////////
test.cpp

#include "test.h"

#include <utility>
#include <iostream>

test::test()
{
}
test::test(const test& mytest)
{
tests=mytest.tests;
}

test& test:perator =(const test &ts)
{
if(this!=&ts)
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();

tests=ts.tests;
}
return *this;
}

test::~test()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();
}
void test::AddMyclass(const string &name,const myclass &my)
{
map<string,myclass*>::iterator ii;
myclass* newmyclass=NULL;

ii=tests.find(name);
if(ii==tests.end())
{
newmyclass=new myclass(my);
tests[name]=newmyclass;
}
}

void test::GetTest()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();ii++)
{
cout<<"the name is:"<<ii->first<<endl;
ii->second->GetMyclass();
}
}

////////////////////////////////////////////////////////////////////
call.h

#ifndef CALL_H_
#define CALL_H_

#include "test.h"

class call
{
public:
inline void SetTest(const test& ts){testagain=ts;}

protected:
test testagain;
};
#endif

///////////////////////////////////////////////////////////////////
main
#include "myclass.h"
#include "test.h"
#include "call.h"

int main()
{
test tt;
myclass my;
string ss[3]={"a","b","c"};
call ca;

for(int i=0;i<3;i++)
my.AddName(ss);

my.GetMyclass();

tt.AddMyclass("my class",my);

tt.GetTest();

ca.SetTest(tt);
return 0;
}

The problem is at the line ca.SetTest(tt). Could somebody tell me how
to solve it? Thanks.

 

[mlimber]

Hi all,

I posted my question two days ago, and tried to solve this problem.
but until now I didn't solve that. and I cut my codes so maybe this
time it is more readable.

/////////////////////////////////////////////////////
#ifndef MYCLASS_H_
#define MYCLASS_H_

#include <string>
#include <list>
//#include "name.h"

using namespace std;
class myclass
{
protected:
list<string> namelist;
// map<int,vector<name> >names;
public:

myclass();
myclass(const myclass &my);
~myclass();
// myclass& operator=(const myclass &it);
void AddName(const string &name);
void GetMyclass();

};

#endif

///////////////////////////////////////////////
myclass.cpp
#include "myclass.h"
#include <algorithm>
#include <iostream>

myclass::myclass()
{

}

myclass::myclass(const myclass &my)
{

namelist=my.namelist;

}

myclass::~myclass()
{

namelist.erase(namelist.begin(),namelist.end());

}

void myclass::AddName(const string &name)
{
list<string>::iterator ii;
ii=find(namelist.begin(),namelist.end(),name);

if(ii==namelist.end())
namelist.push_back(name);

}

void myclass::GetMyclass()
{
list<string>::iterator ii;
for(ii=namelist.begin();ii!=namelist.end();ii++)
cout<<*ii<<endl;

}

/////////////////////////////////////////////////
test.h
#ifndef TEST_H_
#define TEST_H_

#include <string>
#include <map>
#include "myclass.h"

using namespace std;

class test
{
protected:
map<string,myclass*> tests;

public:
test();
test(const test& mytest);
test& operator=(const test& ts);
~test();
void AddMyclass(const string &name,const myclass &my);
void GetTest();

};

#endif

////////////////////////////////////////////////////
test.cpp

#include "test.h"

#include <utility>
#include <iostream>

test::test()
{}

test::test(const test& mytest)
{
tests=mytest.tests;

}

test& test:perator =(const test &ts)
{
if(this!=&ts)
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();

tests=ts.tests;
}
return *this;

}

test::~test()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();}

void test::AddMyclass(const string &name,const myclass &my)
{
map<string,myclass*>::iterator ii;
myclass* newmyclass=NULL;

ii=tests.find(name);
if(ii==tests.end())
{
newmyclass=new myclass(my);
tests[name]=newmyclass;
}

}

void test::GetTest()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();ii++)
{
cout<<"the name is:"<<ii->first<<endl;
ii->second->GetMyclass();
}

}

////////////////////////////////////////////////////////////////////
call.h

#ifndef CALL_H_
#define CALL_H_

#include "test.h"

class call
{
public:
inline void SetTest(const test& ts){testagain=ts;}

Here you make a copy of test. There are now two objects in existence,
so two destructors will be called at some point.

protected:
test testagain;};

#endif

///////////////////////////////////////////////////////////////////
main
#include "myclass.h"
#include "test.h"
#include "call.h"

int main()
{
test tt;
myclass my;
string ss[3]={"a","b","c"};
call ca;

for(int i=0;i<3;i++)
my.AddName(ss);

my.GetMyclass();

tt.AddMyclass("my class",my);

tt.GetTest();

ca.SetTest(tt);
return 0;

}

The problem is at the line ca.SetTest(tt). Could somebody tell me how
to solve it? Thanks.

You could pass ownership of the test object rather than making a copy.
One way to do that is to use std::auto_ptr, which signifies unique
ownership. So if your "call" class accepts an auto_ptr<test>, the
implicit understanding is that it takes sole ownership of that object.

Cheers! --M

 

[Jim Langston]

Hi all,

I posted my question two days ago, and tried to solve this problem.
but until now I didn't solve that. and I cut my codes so maybe this
time it is more readable.

/////////////////////////////////////////////////////
#ifndef MYCLASS_H_
#define MYCLASS_H_

#include <string>
#include <list>
//#include "name.h"

using namespace std;
class myclass
{
protected:
list<string> namelist;
// map<int,vector<name> >names;
public:

myclass();
myclass(const myclass &my);
~myclass();
// myclass& operator=(const myclass &it);
void AddName(const string &name);
void GetMyclass();
};

#endif

///////////////////////////////////////////////
myclass.cpp
#include "myclass.h"
#include <algorithm>
#include <iostream>

myclass::myclass()
{

}

myclass::myclass(const myclass &my)
{

namelist=my.namelist;
}

myclass::~myclass()
{

namelist.erase(namelist.begin(),namelist.end());
}


void myclass::AddName(const string &name)
{
list<string>::iterator ii;
ii=find(namelist.begin(),namelist.end(),name);

if(ii==namelist.end())
namelist.push_back(name);
}

void myclass::GetMyclass()
{
list<string>::iterator ii;
for(ii=namelist.begin();ii!=namelist.end();ii++)
cout<<*ii<<endl;
}

/////////////////////////////////////////////////
test.h
#ifndef TEST_H_
#define TEST_H_

#include <string>
#include <map>
#include "myclass.h"

using namespace std;

class test
{
protected:
map<string,myclass*> tests;

public:
test();
test(const test& mytest);
test& operator=(const test& ts);
~test();
void AddMyclass(const string &name,const myclass &my);
void GetTest();

};

#endif

////////////////////////////////////////////////////
test.cpp

#include "test.h"

#include <utility>
#include <iostream>

test::test()
{
}
test::test(const test& mytest)
{
tests=mytest.tests;
}

Above is your copy constructor. Yet you have a map of pointers you are not
copying correctly. Remember, your test destructor will delete the pointers,
and generally I've found that when a copy constructor is called there is a
temporary made somewhere that needs to be deleted. Pointers moving from
class to class is a bad idea where there is a delete. Generally what you
have to do is duplicate the objects the pointers are pointing to. new a new
pointer, assign the value so that when the old one gets deleted you don't
lose the data.

If I have a class that has pointers to newed objects, I always make the copy
and assignment operators private so they can't be called. Unless you work
with smart pointers you'll have issues.

Think about your line later:
ca.SetTest(tt);
ca is to have a map of the ponters, but so is tt. Yet you only newed them
once. You have 2 different objects (attempting to) point to the same
memory, but they get deleted. If you have 2 objects with pointers, for an
assignment or copy you have to copy the objects the memory is pointing to so
you have 2 seperate memories they are pointing to and their deletes work
correctly, or you have to think very very carefully about object ownership.
After cs.SetTest(tt) who do you expect to "own" the memory the pointers are
pointing to? You can't have 2 seperate instances owning the same pointers.
Unless you use smart pointers of some type.

You have a design flaw here and you are going to have to rethink your
classes.

test& test:perator =(const test &ts)
{
if(this!=&ts)
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();

tests=ts.tests;
}
return *this;
}


test::~test()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();
}
void test::AddMyclass(const string &name,const myclass &my)
{
map<string,myclass*>::iterator ii;
myclass* newmyclass=NULL;

ii=tests.find(name);
if(ii==tests.end())
{
newmyclass=new myclass(my);
tests[name]=newmyclass;
}
}

void test::GetTest()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();ii++)
{
cout<<"the name is:"<<ii->first<<endl;
ii->second->GetMyclass();
}
}

////////////////////////////////////////////////////////////////////
call.h

#ifndef CALL_H_
#define CALL_H_

#include "test.h"

class call
{
public:
inline void SetTest(const test& ts){testagain=ts;}

protected:
test testagain;
};
#endif

///////////////////////////////////////////////////////////////////
main
#include "myclass.h"
#include "test.h"
#include "call.h"

int main()
{
test tt;
myclass my;
string ss[3]={"a","b","c"};
call ca;

for(int i=0;i<3;i++)
my.AddName(ss);

my.GetMyclass();

tt.AddMyclass("my class",my);

tt.GetTest();

ca.SetTest(tt);
return 0;
}

The problem is at the line ca.SetTest(tt). Could somebody tell me how
to solve it? Thanks.

 

[Roland Pibinger]

int main()
{
test tt;
myclass my;
string ss[3]={"a","b","c"};
call ca;

for(int i=0;i<3;i++)
my.AddName(ss);

my.GetMyclass();

tt.AddMyclass("my class",my);

tt.GetTest();

ca.SetTest(tt);
return 0;

}

The problem is at the line ca.SetTest(tt). Could somebody tell me how
to solve it? Thanks.

You could pass ownership of the test object rather than making a copy.
One way to do that is to use std::auto_ptr, which signifies unique
ownership. So if your "call" class accepts an auto_ptr<test>, the
implicit understanding is that it takes sole ownership of that object.

Goodness gracious! tt is a local automatic object. No need to "pass
ownership" or use a 'smart pointer'. Better make the copy constructor
and operator= for all classes private (leave them un-implemented) and
your code will work with some changes.

Best regards,
Roland Pibinger

 

[Fei Liu]

Hi all,

I posted my question two days ago, and tried to solve this problem.
but until now I didn't solve that. and I cut my codes so maybe this
time it is more readable.

/////////////////////////////////////////////////////
#ifndef MYCLASS_H_
#define MYCLASS_H_

#include <string>
#include <list>
//#include "name.h"

using namespace std;
class myclass
{
protected:
list<string> namelist;
// map<int,vector<name> >names;
public:

myclass();
myclass(const myclass &my);
~myclass();
// myclass& operator=(const myclass &it);
void AddName(const string &name);
void GetMyclass();
};

#endif

///////////////////////////////////////////////
myclass.cpp
#include "myclass.h"
#include <algorithm>
#include <iostream>

myclass::myclass()
{

}

myclass::myclass(const myclass &my)
{

namelist=my.namelist;
}

myclass::~myclass()
{

namelist.erase(namelist.begin(),namelist.end());
}


void myclass::AddName(const string &name)
{
list<string>::iterator ii;
ii=find(namelist.begin(),namelist.end(),name);

if(ii==namelist.end())
namelist.push_back(name);
}

void myclass::GetMyclass()
{
list<string>::iterator ii;
for(ii=namelist.begin();ii!=namelist.end();ii++)
cout<<*ii<<endl;
}

/////////////////////////////////////////////////
test.h
#ifndef TEST_H_
#define TEST_H_

#include <string>
#include <map>
#include "myclass.h"

using namespace std;

class test
{
protected:
map<string,myclass*> tests;

public:
test();
test(const test& mytest);
test& operator=(const test& ts);
~test();
void AddMyclass(const string &name,const myclass &my);
void GetTest();

};

#endif

////////////////////////////////////////////////////
test.cpp

#include "test.h"

#include <utility>
#include <iostream>

test::test()
{
}
test::test(const test& mytest)
{
tests=mytest.tests;
}

test& test:perator =(const test &ts)
{
if(this!=&ts)
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();

tests=ts.tests;
}
return *this;
}

test::~test()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();++ii)
delete(ii->second);
tests.clear();
}
void test::AddMyclass(const string &name,const myclass &my)
{
map<string,myclass*>::iterator ii;
myclass* newmyclass=NULL;

ii=tests.find(name);
if(ii==tests.end())
{
newmyclass=new myclass(my);
tests[name]=newmyclass;
}
}

void test::GetTest()
{
map<string,myclass*>::iterator ii;
for(ii=tests.begin();ii!=tests.end();ii++)
{
cout<<"the name is:"<<ii->first<<endl;
ii->second->GetMyclass();
}
}

////////////////////////////////////////////////////////////////////
call.h

#ifndef CALL_H_
#define CALL_H_

#include "test.h"

class call
{
public:
inline void SetTest(const test& ts){testagain=ts;}

protected:
test testagain;
};
#endif

///////////////////////////////////////////////////////////////////
main
#include "myclass.h"
#include "test.h"
#include "call.h"

int main()
{
test tt;
myclass my;
string ss[3]={"a","b","c"};
call ca;

for(int i=0;i<3;i++)
my.AddName(ss);

my.GetMyclass();

tt.AddMyclass("my class",my);

tt.GetTest();

ca.SetTest(tt);
return 0;
}

The problem is at the line ca.SetTest(tt). Could somebody tell me how
to solve it? Thanks.

Your copy constructor hints a shallow copy while your copy assignment
operator hints a deep copy. You are contradicting youself in terms of
exactly what you want to do with the private data in test class.

Fei

 

[mlimber]

Goodness gracious! tt is a local automatic object. No need to "pass
ownership" or use a 'smart pointer'. Better make the copy constructor
and operator= for all classes private (leave them un-implemented) and
your code will work with some changes.

Like I said, there are multiple ways to accomplish this, and passing
ownership like I described is one common and useful way to do it.
Compare this FAQ by the Creator on using auto_ptr to pass ownership:

http://www.research.att.com/~bs/bs_faq2.html#memory-leaks

Of course, the "Big Three" (or probably better, the "Big Two" --
http://www.artima.com/cppsource/bigtwo.html) problems ought to be
fixed in any case.

Cheers! --M

 

[David]

Like I said, there are multiple ways to accomplish this, and passing
ownership like I described is one common and useful way to do it.
Compare this FAQ by the Creator on using auto_ptr to pass ownership:

http://www.research.att.com/~bs/bs_faq2.html#memory-leaks

Of course, the "Big Three" (or probably better, the "Big Two" --http://www.artima.com/cppsource/bigtwo.html) problems ought to be
fixed in any case.

Cheers! --M

Thanks all for your help. I think I need a "deep copy" in my
copyconstructor. sorry I am new in C++, so when I tried to do below,
there was something wrong with this:
test::test(const test& mytest)
{

map<string,myclass*>::iterator ii;

for(ii=mytest.tests.begin();ii!=mytest.tests.end();++ii)
{
myclass* my=new myclass;
memcpy(my,ii->second,sizeof(ii->second));
tests.insert(make_pair(ii->first,my));
}


}

there was an error: "c:\Ug\Solution1\test\test.cpp(16): error C2679:
binary '=' : no operator found which takes a right-hand operand of
type 'std::_Tree<_Traits>::const_iterator' " at the for loop point.
can anybody tell me why and what's the right way to do the copy
constructor? thanks

 

[Jim Langston]

Thanks all for your help. I think I need a "deep copy" in my
copyconstructor. sorry I am new in C++, so when I tried to do below,
there was something wrong with this:
test::test(const test& mytest)
{

map<string,myclass*>::iterator ii;

for(ii=mytest.tests.begin();ii!=mytest.tests.end();++ii)
{
myclass* my=new myclass;
memcpy(my,ii->second,sizeof(ii->second));
tests.insert(make_pair(ii->first,my));
}


}

there was an error: "c:\Ug\Solution1\test\test.cpp(16): error C2679:
binary '=' : no operator found which takes a right-hand operand of
type 'std::_Tree<_Traits>::const_iterator' " at the for loop point.
can anybody tell me why and what's the right way to do the copy
constructor? thanks

The parameter to the copy constructor is a const test& mytest. mytest is
constant. You are trying to use a non-constant iterator on it, which isn't
allowed.
Try:

map<string,myclass*>::const_iterator ii;

 

[mlimber]

Thanks all for your help. I think I need a "deep copy" in my
copyconstructor. sorry I am new in C++, so when I tried to do below,
there was something wrong with this:
test::test(const test& mytest)
{

map<string,myclass*>::iterator ii;

You need

map<string,myclass*>::const_iterator ii;

since mytest is const.

for(ii=mytest.tests.begin();ii!=mytest.tests.end();++ii)
{
myclass* my=new myclass;
memcpy(my,ii->second,sizeof(ii->second));
tests.insert(make_pair(ii->first,my));
}

}

there was an error: "c:\Ug\Solution1\test\test.cpp(16): error C2679:
binary '=' : no operator found which takes a right-hand operand of
type 'std::_Tree<_Traits>::const_iterator' " at the for loop point.
can anybody tell me why and what's the right way to do the copy
constructor? thanks

Read the article on the law of the Big Two that I gave in my previous
post.

Here's a tip: If you see memcpy in C++ code, that's bad (usually).
Prefer to give myclass proper copy semantics so you can say:

*my = *( ii->second );

Better still would be to use a smart pointer that does -- depending on
your needs -- either reference counting (e.g., std::tr1::shared_ptr,
boost::shared_ptr, Loki::SmartPtr, or the one in FAQ 16.22 and
following) or deep copying (e.g., Loki::SmartPtr) for you
automatically.

Cheers! --M

 

[Roland Pibinger]

I think I need a "deep copy" in my copyconstructor.

You need no copy constructor at all. You can and, IMO, should
implement your library with disabled (= private) copy constructors and
assignmet operators. There is no need to duplicate and dynamically
allocate objects.

Best wishes,
Roland Pibinger