Segmentation fault in list...

[saratoga]

Hi, all!
I've 'Segmentation fault' in this code. I've found that there are two
calls to destructor.

TestSTL.h:
#include <list>
#include <string>
#include <iostream>

using namespace std;

class TestSTL
{
string *_str;

public:
TestSTL() { _str = new string("test"); };
~TestSTL() { cerr << "destruct" << endl; delete _str; }
};


TestSTL.cpp:
#include "TestSTL.h"

int main()
{
list<TestSTL> lst;
lst.push_back(TestSTL());
}

Could U explain to me what's wrong here?

Thanx.
With the best regards
Daniel.

 

[Thomas Tutone]

Hi, all!
I've 'Segmentation fault' in this code. I've found that there are two
calls to destructor.

TestSTL.h:
#include <list>
#include <string>
#include <iostream>

using namespace std;

class TestSTL
{
string *_str;

public:
TestSTL() { _str = new string("test"); };
~TestSTL() { cerr << "destruct" << endl; delete _str; }
};


TestSTL.cpp:
#include "TestSTL.h"

int main()
{
list<TestSTL> lst;
lst.push_back(TestSTL());
}

Could U explain to me what's wrong here?

You failed to define an appropriate copy constructor. The
compiler-generated copy constructor is generally not appropriate for
classes (like yours) that acquire resources (in this case, by using
new).

Best regards,

Tom

 

[Ron Natalie]

You failed to define an appropriate copy constructor. The
compiler-generated copy constructor is generally not appropriate for
classes (like yours) that acquire resources (in this case, by using
new).

And an copy-assignment operator. Std containers require both.

 

[Ram]

A side comment- Its not a good idea to say something like new string
(unless u are allocating an array of strings). Using std::string can
relieve u of having to take care of memory management.

Best regards
Ram

 

[Csaba]

You failed to define an appropriate copy constructor. The
compiler-generated copy constructor is generally not appropriate for
classes (like yours) that acquire resources (in this case, by using
new).

For the OP:

When the code does push_lst.back, it creates a *copy* of the TestSTL
object you passed (the nameless temporary). Because you didn't specify a
copy constructor, the compiler generated one for you. This compiler-
generated copy constructor does a member-by-member copy (also called
"shallow copy"). So now you have two TestSTL objects, and their _str
members point to the same dynamically allocated string. The object you
created (a temporary) will be destroyed at the end of the push_back
statement, and it will deallocate the string. When the list is destroyed,
it will destroy the stored TestSTL object, which in turn will try to
deallocate the string again.

If you want to store objects in an STL container, it's best to avoid
naked pointer members. If you need a string, you can use std::string. If
you need a lump of memory, you can use std::vector<char>. All STL classes
have proper value semantics; they will take care of the copy operations
that permeate C++.

If you want to keep the naked pointer and still use std::list, you have
two possibilities:

1) Implement the copy constructor and copy assignment operator yourself
to perform a deep copy (allocate another string and copy the original).
This is what std::string does (hint, hint).

2) Do some sort of reference counting.

 

[Puppet_Sock]

I've 'Segmentation fault' in this code. I've found that there are two
calls to destructor.

[snip]

Others have given the specific answer to your question.
For this, and many related questions, you should check
out the FAQ for the group.

http://www.parashift.com/c++-faq-lite/
Socks