A problem when using a reference to a vector of a class with a pointer member to an array

[Brian]

Dear Programmers,

I have a class with a pointer to an array. In the destructor, I just
freed this pointer. A problem happens if I define a reference to a
vector of this kind of class. The destruction of the assigned memory
seems to call the class destructor more than once. I don't know the
reason or whether I used the vector class correctly. Attached is my
program. Thanks for your help.

Regards,
Brian

Run Result:
$ ./a.out
done
*** glibc detected *** double free or corruption (top): 0x08fa40a0 ***
Aborted

Code:
#include <iostream>
#include <vector>

using namespace std;

class ArrayWrap
{
public:
char * pt_ch;
ArrayWrap()
{
pt_ch = new char[100];
}
~ArrayWrap() {
delete[] pt_ch; // if delete this statement, no error then,
// but not a reasonable design
}
};

int main()
{
ArrayWrap dum;
ArrayWrap & rdum = dum;
vector<ArrayWrap> dummy(10);
vector<ArrayWrap> & refdummy = dummy;

cout << "done" << endl;
return 0;
}

 

[mlimber]

Dear Programmers,

I have a class with a pointer to an array. In the destructor, I just
freed this pointer. A problem happens if I define a reference to a
vector of this kind of class. The destruction of the assigned memory
seems to call the class destructor more than once. I don't know the
reason or whether I used the vector class correctly. Attached is my
program. Thanks for your help.

Regards,
Brian

Run Result:
$ ./a.out
done
*** glibc detected *** double free or corruption (top): 0x08fa40a0 ***
Aborted

Code:
#include <iostream>
#include <vector>

using namespace std;

class ArrayWrap
{
public:
char * pt_ch;
ArrayWrap()
{
pt_ch = new char[100];
}
~ArrayWrap() {
delete[] pt_ch; // if delete this statement, no error then,
// but not a reasonable design
}
};

int main()
{
ArrayWrap dum;
ArrayWrap & rdum = dum;
vector<ArrayWrap> dummy(10);
vector<ArrayWrap> & refdummy = dummy;

cout << "done" << endl;
return 0;
}

Your wrapper class needs a copy constructor. See the third bullet about
the law of the big three here:

http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.10

But, why not just use vector< vector<char> > or vector<string> (see
http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1)?

Cheers! --M

 

[Brian]

Thanks a lot. It works. This is a simplified program from my real
work to reproduce the problem. We can't use vector< vector<char> > or
vector<string> because we need to write what the pointer points to as a
whole body to a tape in one unbuffered write() for speed concern. I
don't know any better way.

However, I still don't understand the reason for the need of copy
constructor. During the lunch, I came out of the idea that why I don't
just use a pointer to the established vector in stead of a reference.
To my surprise, it doesn't work either without a copy constructor. I
don't get it. I didn't make any copy, but just made a pointer or a
reference referring to the established vector object. I don't see
where a copy was made. I didn't put anything in the copy constructor
and it still works. Is this a requirement that the standard library
has? Are there some hidden copy operations made when we define a
vector reference or pointer?

Thanks,
Brian

 

[Jim Langston]

Thanks a lot. It works. This is a simplified program from my real
work to reproduce the problem. We can't use vector< vector<char> > or
vector<string> because we need to write what the pointer points to as a
whole body to a tape in one unbuffered write() for speed concern. I
don't know any better way.

However, I still don't understand the reason for the need of copy
constructor. During the lunch, I came out of the idea that why I don't
just use a pointer to the established vector in stead of a reference.
To my surprise, it doesn't work either without a copy constructor. I
don't get it. I didn't make any copy, but just made a pointer or a
reference referring to the established vector object. I don't see
where a copy was made. I didn't put anything in the copy constructor
and it still works. Is this a requirement that the standard library
has? Are there some hidden copy operations made when we define a
vector reference or pointer?

Thanks,
Brian

Please don't top post. Please supply more context from previous post.

int main()
{
ArrayWrap dum;
ArrayWrap & rdum = dum;
vector<ArrayWrap> dummy(10);
// I believe the previous line is your problem

vector<ArrayWrap> & refdummy = dummy;

cout << "done" << endl;
return 0;
}

Vectors tend to be a pain this way. It seems (to me) that when you put an
item into a vector it first creates it into a temporary, then copies the
temporary into the vector. So when the temporary gets destroyed, it runs
the destructor and you're hozed.

I've run into this problem before with classes that are not copyable (
unique objects loaded that can't be copied, or are just a waste of time to
copy). The way I've solved this is to make a private copy constructor (so
it *can't* be copied) then to make my vector into pointers.

vector<ArrayWrap*> dummy;
for ( i = 0; i < 10; i++ )
ArrayWrap.push_back( new ArrayWrap );
// code

for ( vector<ArrayWrap>::iterator it = ArrayWrap.begin(); it !=
ArrayWrap.end(); ++it )
delete (*it); // double check syntax of this

 

[BobR]

Jim Langston wrote in message ...

vector<ArrayWrap*> dummy;
for ( i = 0; i < 10; i++ )

Got a headache today, Jim? Did you mean?

// > ArrayWrap.push_back( new ArrayWrap );
dummy.push_back( new ArrayWrap );

// code

// >for ( vector<ArrayWrap>::iterator it = ArrayWrap.begin(); it !=
// >ArrayWrap.end(); ++it )

delete (*it); // double check syntax of this

Yep, looks same as one I use:
// ------------------------------------
template<class Seq> void PurgeCon( Seq &cont ){
typename Seq::iterator it;
for( it = cont.begin(); it != cont.end(); ++it ){
delete *it;
*it = 0; // just in case it's called twice.
} // for(it)
} // template<class Seq> void PurgeCon(Seq&)
// ------------------------------------

PurgeCon( dummy );

 

[Jim Langston]

Jim Langston wrote in message ...

Got a headache today, Jim? Did you mean?

// > ArrayWrap.push_back( new ArrayWrap );
dummy.push_back( new ArrayWrap );

// <G> Been there, done that. <G>

Heh, yeah, nice catch.

// >for ( vector<ArrayWrap>::iterator it = ArrayWrap.begin(); it !=
// >ArrayWrap.end(); ++it )
for( vector<ArrayWrap*>::iterator it( dummy.begin() ); it !=
dummy.end(); ++it )

Me and ArrayWrap again instead of dummy. lo.

I notice you use it (dummy.begin()) to construct the iterator. I've never
known I could do this, but does it really save anything?

Yep, looks same as one I use:
// ------------------------------------
template<class Seq> void PurgeCon( Seq &cont ){
typename Seq::iterator it;
for( it = cont.begin(); it != cont.end(); ++it ){
delete *it;

Yet, here you don't use the initialization, why?

 

[Paul]

Thanks a lot. It works. This is a simplified program from my real
work to reproduce the problem. We can't use vector< vector<char> > or
vector<string> because we need to write what the pointer points to as a
whole body to a tape in one unbuffered write() for speed concern. I
don't know any better way.

However, I still don't understand the reason for the need of copy

constructor.

Not understanding what you really did (please post your updated class), the
vector requires that your object are copyable, and to make sure no bugs
exist, the objects should be able to be copied safely with no issues. Your
original ArrayWrap class does not satisfy the "safely copyable" requirement
(although it is copyable).

int main()
{
ArrayWrap a;
ArrayWrap b = a;
}

Try this with your original class, or with the class that you have changed
it to. Does this work correctly (i.e. no memory leaks, crashes, double
deletion errors, etc.)? It won't work with your original class, since at
the end of main(), a double deletion error will occur when object a and b
are destroyed.

That's simply what vector is doing, and if there are bugs with the simple
code above, then there will be bugs when vector gets its hands on it.

- Paul

 

[Gavin Deane]

Thanks a lot. It works. This is a simplified program from my real
work to reproduce the problem. We can't use vector< vector<char> > or
vector<string> because we need to write what the pointer points to as a
whole body to a tape in one unbuffered write() for speed concern. I
don't know any better way.

When you say "what the pointer points to" are you talking about the
pointer in your ArrayWrap class? If so then what you need does not
preclude you replacing ArrayWrap with vector<char> and replacing
vector<ArrayWrap> with vector<vector<char> >. "What the pointer points
to" in your ArrayWrap class becomes "the contents of a vector<char>".
You can very easily get at the contents of a vector<char> for your
single unbuffered write.

If I have misunderstood you when you say "what the pointer points to"
please clarify as there may still be a better solution.

Gavin Deane

 

[BobR]

Jim Langston wrote in message ...

"BobR" wrote in message

Heh, yeah, nice catch.


Me and ArrayWrap again instead of dummy. lo.

I notice you use it (dummy.begin()) to construct the iterator. I've never
known I could do this, but does it really save anything?

Sure, if you did it a million times in your program and optimazation was off,
it could save a second in runtime! <G> Actually, I was just being a
smart-ass.
If you are used to assignment (=), you should be consistant and always use
assignment in your code. Mostly a 'style choice'.

You seldom see:

class Mine{.....};
Mine My = 12345;
.....but, usually:
Mine My(12345);

.....so....

int Num(12345);

Yet, here you don't use the initialization, why?

Uuuh, old code? Stole the code from someone and didn't change it?

for( typename Seq::iterator it( cont.begin() ); it != cont.end(); ++it){

// typename Seq::iterator it( cont.begin() );
// for( ; it != cont.end(); ++it ){ delete *it; *it = 0; }

Newbies, see:
"Thinking In C++", Volume 2: Practical Programming (Bruce Eckel, Chuck
Allison)
Chap. 5: Templates in Depth (sub "The typename keyword")
http://www.mindview.net/Books/TICPP/ThinkingInCPP2e.html
.....for why the 'typename' is needed. (or look it up in *your* book)


[ I'm not a expert, feel free to correct me. ]

 

[Brian]

Dear Jim,

Thank you very much. Your explanation is right. It was not the
reference or pointer that caused the problem but the place your pointed
out.

The vector constructor does create a temporary object and copy it to
all the vector members and destroy it. I put some cout statements in
the constructor, copy constructor and destructor and got relevant
information.

I said that when I put nothing in the copy constructor it still works.
I was wrong. It only works when I don't access anything within the
object, otherwise it causes segmentation error because all its members
will have no valid value if nothing was done in the copy constructor.

I attached my updated program below for context and left some comments.
Now I don't need your technique of pointers. I will keep it in mind
in case needed. Thanks a lot.

Regards,
Brian

Please don't top post. Please supply more context from previous post.

int main()
{
ArrayWrap dum;
ArrayWrap & rdum = dum;
vector<ArrayWrap> dummy(10);
// I believe the previous line is your problem

vector<ArrayWrap> & refdummy = dummy;

cout << "done" << endl;
return 0;
}

Vectors tend to be a pain this way. It seems (to me) that when you put an
item into a vector it first creates it into a temporary, then copies the
temporary into the vector. So when the temporary gets destroyed, it runs
the destructor and you're hozed.

I've run into this problem before with classes that are not copyable (
unique objects loaded that can't be copied, or are just a waste of time to
copy). The way I've solved this is to make a private copy constructor (so
it *can't* be copied) then to make my vector into pointers.

vector<ArrayWrap*> dummy;
for ( i = 0; i < 10; i++ )
ArrayWrap.push_back( new ArrayWrap );
// code

for ( vector<ArrayWrap>::iterator it = ArrayWrap.begin(); it !=
ArrayWrap.end(); ++it )
delete (*it); // double check syntax of this

------------------------------------------------------------------------------------------------------
#include <iostream>
#include <vector>

using namespace std;

class ArrayWrap
{
public:
char * pt_ch;
ArrayWrap()
{
pt_ch = new char[100];
pt_ch[0] = 'b';
cout << "create dummy" << endl;
}

ArrayWrap(const ArrayWrap & AnArray)
{
pt_ch = new char[100]; // this made copy's pt_ch valid
pt_ch[0] = AnArray.pt_ch[0]; // this assigned values to
// the copy.
cout << "copy dummy" << endl;
}

~ArrayWrap()
{
delete[] pt_ch;
cout << "destruct dummy" << endl;
}

int print()
{
cout << pt_ch[0] << endl;
return 1;
}
};

int main()
{
ArrayWrap dum;
vector<ArrayWrap> dummy(5); // copy constructor
// is needed here, otherwise
// pt_ch of dummy points
// to destroyed temporary
// ArrayWrap object
vector<ArrayWrap> & refdummy = dummy;
vector<ArrayWrap> * pdummy;
dummy[0].print(); // this will cause segmentation
// error if copy constructors
// first two statements is omitted
cout << "done" << endl;
return 0;
}

 

[BobR]

Brian wrote in message ...

A logical next step could be to get rid of the "magic numbers".


const int bufsize = 100;
// int const bufsize(100); // another way to write it.

Now, if you decide to change the size of your 'char array', you only need to
change it in one place and it's all taken care of for you. (see changes
below)

class ArrayWrap{
public:
char * pt_ch;
ArrayWrap(){

// pt_ch = new char[100];
pt_ch = new char[ bufsize ];

pt_ch[0] = 'b';
cout << "create dummy" << endl;
}

ArrayWrap(const ArrayWrap & AnArray){

// pt_ch = new char[100]; // this made copy's pt_ch valid
pt_ch = new char[ bufsize ]; // this made copy's pt_ch valid

pt_ch[0] = AnArray.pt_ch[0]; // this assigned values to
// the copy.

// Since you declared an 'const int' for the buffer size, you could copy
// the whole thing (not just one char) without doing any other 'checks':

for( int i(0); i < bufsize; ++i){ // simple example
pt_ch[ i ] = AnArray.pt_ch[ i ];
} // for(i)
// [ look up 'std::copy', 'memcpy', etc., for other ways. ]

cout << "copy dummy" << endl;
}

~ArrayWrap(){
delete[] pt_ch;
cout << "destruct dummy" << endl;
}

int print()
{
cout << pt_ch[0] << endl;
return 1;
}
};

int main(){
ArrayWrap dum;

If you don't use 'dum', remove that instance. etc..

vector<ArrayWrap> dummy(5); // copy constructor
// is needed here, otherwise
// pt_ch of dummy points
// to destroyed temporary
// ArrayWrap object
vector<ArrayWrap> & refdummy = dummy;
vector<ArrayWrap> * pdummy;
dummy[0].print(); // this will cause segmentation
// error if copy constructors
// first two statements is omitted
cout << "done" << endl;
return 0;
}

 

[Brian]

Hi, Gavin,

Sorry for the late reply. I did mean the pointer in my ArrayWrap class
here, which is "pt_ch" in my original program. To my knowledge, we can
only access each member of a vector one by one in stead of a whole
body. How can I use one write to write all values in a vector to a
file?

I attached my old program in case it's lost.

Thanks,
Brian

When you say "what the pointer points to" are you talking about the
pointer in your ArrayWrap class? If so then what you need does not
preclude you replacing ArrayWrap with vector<char> and replacing
vector<ArrayWrap> with vector<vector<char> >. "What the pointer points
to" in your ArrayWrap class becomes "the contents of a vector<char>".
You can very easily get at the contents of a vector<char> for your
single unbuffered write.

If I have misunderstood you when you say "what the pointer points to"
please clarify as there may still be a better solution.

Gavin Deane

Code:
#include <iostream>
#include <vector>

using namespace std;

class ArrayWrap
{
public:
char * pt_ch;
ArrayWrap()
{
pt_ch = new char[100];
}
~ArrayWrap() {
delete[] pt_ch; // if delete this statement, no error then,
// but not a reasonable design
}

};

int main()
{
ArrayWrap dum;
ArrayWrap & rdum = dum;
vector<ArrayWrap> dummy(10);
vector<ArrayWrap> & refdummy = dummy;

cout << "done" << endl;
return 0;
}