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

Discussion in 'C++' started by Brian, Oct 19, 2006.

  1. Brian

    Brian Guest

    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;
    }
     
    Brian, Oct 19, 2006
    #1
    1. Advertising

  2. Brian

    mlimber Guest

    Brian wrote:
    > 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
     
    mlimber, Oct 19, 2006
    #2
    1. Advertising

  3. Brian

    Brian Guest

    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

    mlimber wrote:
    > 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, Oct 19, 2006
    #3
  4. Brian

    Jim Langston Guest

    "Brian" <> wrote in message
    news:...
    > 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
    >
    > mlimber wrote:
    >> 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


    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
     
    Jim Langston, Oct 19, 2006
    #4
  5. Brian

    BobR Guest

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

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

    >// code
    >

    // >for ( vector<ArrayWrap>::iterator it = ArrayWrap.begin(); it !=
    // >ArrayWrap.end(); ++it )
    for( vector<ArrayWrap*>::iterator it( dummy.begin() ); it !=
    dummy.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 );

    --
    Bob R
    POVrookie
     
    BobR, Oct 20, 2006
    #5
  6. Brian

    Jim Langston Guest

    "BobR" <> wrote in message
    news:4cWZg.314319$...
    >
    > 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 );
    >
    > // <G> Been there, done that. <G>


    Heh, yeah, nice catch.

    >>// code
    >>

    > // >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?

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


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

    > *it = 0; // just in case it's called twice.
    > } // for(it)
    > } // template<class Seq> void PurgeCon(Seq&)
    > // ------------------------------------
    >
    > PurgeCon( dummy );
     
    Jim Langston, Oct 20, 2006
    #6
  7. Brian

    Paul Guest

    "Brian" <> wrote in message
    news:...
    > 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
     
    Paul, Oct 20, 2006
    #7
  8. Brian

    Gavin Deane Guest

    Brian wrote:
    > 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
     
    Gavin Deane, Oct 20, 2006
    #8
  9. Brian

    BobR Guest

    Jim Langston wrote in message ...
    >"BobR" wrote in message
    >>
    >> 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 );
    >> // <G> Been there, done that. <G>

    >
    >Heh, yeah, nice catch.
    >
    >>>// code
    >>>

    >> // >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?


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

    .... is my choice of style. <G>

    >
    >>
    >>> 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 ){

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

    >> delete *it;
    >> *it = 0; // just in case it's called twice.
    >> } // for(it)
    >> } // template<class Seq> void PurgeCon(Seq&)
    >> // ------------------------------------
    >>
    >> PurgeCon( dummy );



    [ I'm not a expert, feel free to correct me. ]
    --
    Bob R
    POVrookie
     
    BobR, Oct 20, 2006
    #9
  10. Brian

    Brian Guest

    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

    Jim Langston wrote:
    >
    > 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;
    }
     
    Brian, Oct 27, 2006
    #10
  11. Brian

    BobR Guest

    Brian wrote in message ...

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


    But, do trim stuff you are not referencing. <G>


    >----------------------------------------------------------------------------

    --------------------------
    >#include <iostream>
    >#include <vector>
    >using namespace std;


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


    --
    Bob R
    POVrookie
     
    BobR, Oct 28, 2006
    #11
  12. Brian

    Brian Guest

    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

    Gavin Deane wrote:
    > Brian wrote:
    > > 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


    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;
    }
     
    Brian, Nov 13, 2006
    #12
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. ding feng
    Replies:
    8
    Views:
    873
    Dhruv
    Jul 2, 2003
  2. Fraser Ross
    Replies:
    4
    Views:
    1,094
    Fraser Ross
    Aug 14, 2004
  3. Replies:
    8
    Views:
    1,999
    Csaba
    Feb 18, 2006
  4. Stephen Howe
    Replies:
    2
    Views:
    306
    Stephen Howe
    Nov 6, 2012
  5. somenath
    Replies:
    10
    Views:
    311
    James Kanze
    Jul 2, 2013
Loading...

Share This Page