Cleanup Technique

Discussion in 'C++' started by brad.power@gmail.com, Aug 28, 2007.

  1. Guest

    Hi All,

    Often in C++ code I find myself having to allocate character arrays to
    pass back to callers for error codes, or as topic/data in message
    passing/observer type scenarios (When having them on the heap is
    necessary to avoid them going out of scope before something else, like
    another thread, has had a chance to process them). What I wanted was a
    neat way to keep track of these allocations within the class, and
    clean them all up nicely, without too much overhead.

    What I hit upon is based on the following ideas:
    1. std::string is just 'better' than char*, so use it!
    2. std::vector, in its destructor, will attempt to destroy each object
    within it.

    So what I did was to declare a vector of std::strings inside
    'SomeClass' - a class which needs to keep track of a lot of strings it
    creates during its adventures:

    class SomeClass
    {
    ....
    private:
    std::vector<std::string> m_stringvector;
    ....
    }

    And then, in the body of some method in SomeClass:

    std::string* s1 = new std::string();
    std::string* s2 = new std::string();
    //do something with s1 and s2 - i.e. set them and pass them back to a
    function
    //now add them to the class vector to keep track of them:
    m_stringvector.push_back(*s1);
    m_stringvector.push_back(*s2);

    So the idea is that m_stringvector is just a container to keep track
    of all of my allocated strings, and when SomeClass destructs,
    m_stringvector will automatically free all the string objects.

    To me, this seems like a good way to keep track, as long as:
    1. strings created this way are not freed anywhere else
    2. The program design is happy to have all the strings destruct when
    the class destructs
    3. The life of SomeClass is such that it will never end up holding
    zillions of strings, hogging memory
    4. The programmer remembers to push the strings onto the vector :)

    Since I stumbled across this in my own programming, and havent seen it
    in a book or as a documented technique, I wanted to run it past the
    group for any comments you may have. Is it as good an idea as I think?
    Are there any obvious (or not so obvious) gotchas or 'you shouldn't do
    that' aspects to this? Is this an actual documented technique that I
    am just ignorant of?

    Thanks
    Brad
     
    , Aug 28, 2007
    #1
    1. Advertising

  2. Phlip Guest

    wrote:

    > Often in C++ code I find myself having to allocate character arrays to
    > pass back to callers for error codes, or as topic/data in message
    > passing/observer type scenarios (When having them on the heap is
    > necessary to avoid them going out of scope before something else, like
    > another thread, has had a chance to process them). What I wanted was a
    > neat way to keep track of these allocations within the class, and
    > clean them all up nicely, without too much overhead.


    C++ throws objects by copy, so all you need are valid copy semantics.

    > What I hit upon is based on the following ideas:
    > 1. std::string is just 'better' than char*, so use it!


    Always follow that rule, for nearly all strings everywhere.

    > 2. std::vector, in its destructor, will attempt to destroy each object
    > within it.


    Yes, but destroy does not mean delete.

    > std::string* s1 = new std::string();
    > std::string* s2 = new std::string();


    Why the new?

    > //do something with s1 and s2 - i.e. set them and pass them back to a
    > function
    > //now add them to the class vector to keep track of them:
    > m_stringvector.push_back(*s1);
    > m_stringvector.push_back(*s2);


    Make m_stringvector store strings, and just push them in!

    > 1. strings created this way are not freed anywhere else


    Treat the string object as an object; it will take care of freeing its
    controlled array.

    However, your m_stringvector will not natively delete all its strings, so
    they will leak. Again: Just store, copy, and throw strings, not pointers to
    string objects, and let them take care of their own low-level details.

    --
    Phlip
    http://www.oreilly.com/catalog/9780596510657/
    ^ assert_xpath
    http://tinyurl.com/23tlu5 <-- assert_raise_message
     
    Phlip, Aug 28, 2007
    #2
    1. Advertising

  3. * :
    > Hi All,
    >
    > Often in C++ code I find myself having to allocate character arrays to
    > pass back to callers for error codes, or as topic/data in message
    > passing/observer type scenarios (When having them on the heap is
    > necessary to avoid them going out of scope before something else, like
    > another thread, has had a chance to process them). What I wanted was a
    > neat way to keep track of these allocations within the class, and
    > clean them all up nicely, without too much overhead.
    >
    > What I hit upon is based on the following ideas:
    > 1. std::string is just 'better' than char*, so use it!
    > 2. std::vector, in its destructor, will attempt to destroy each object
    > within it.
    >
    > So what I did was to declare a vector of std::strings inside
    > 'SomeClass' - a class which needs to keep track of a lot of strings it
    > creates during its adventures:
    >
    > class SomeClass
    > {
    > ...
    > private:
    > std::vector<std::string> m_stringvector;
    > ...
    > }


    OK so far, except missing semicolon.


    > And then, in the body of some method in SomeClass:
    >
    > std::string* s1 = new std::string();
    > std::string* s2 = new std::string();
    > //do something with s1 and s2 - i.e. set them and pass them back to a
    > function
    > //now add them to the class vector to keep track of them:
    > m_stringvector.push_back(*s1);
    > m_stringvector.push_back(*s2);


    Here you have needless dynamic memory allocation and a memory leak.

    s1 and s2 will not be deallocated.

    Why don't you declare s1 and s2 directly as sd::string.


    Cheers, & hth.,

    - Alf

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    Alf P. Steinbach, Aug 28, 2007
    #3
  4. Guest

    On Aug 28, 10:46 pm, Phlip <> wrote:
    > wrote:
    > > Often in C++ code I find myself having to allocate character arrays to
    > > pass back to callers for error codes, or as topic/data in message
    > > passing/observer type scenarios (When having them on the heap is
    > > necessary to avoid them going out of scope before something else, like
    > > another thread, has had a chance to process them). What I wanted was a
    > > neat way to keep track of these allocations within the class, and
    > > clean them all up nicely, without too much overhead.

    >
    > C++ throws objects by copy, so all you need are valid copy semantics.
    >
    > > What I hit upon is based on the following ideas:
    > > 1. std::string is just 'better' than char*, so use it!

    >
    > Always follow that rule, for nearly all strings everywhere.
    >
    > > 2. std::vector, in its destructor, will attempt to destroy each object
    > > within it.

    >
    > Yes, but destroy does not mean delete.
    >
    > > std::string* s1 = new std::string();
    > > std::string* s2 = new std::string();

    >
    > Why the new?
    >
    > > //do something with s1 and s2 - i.e. set them and pass them back to a
    > > function
    > > //now add them to the class vector to keep track of them:
    > > m_stringvector.push_back(*s1);
    > > m_stringvector.push_back(*s2);

    >
    > Make m_stringvector store strings, and just push them in!
    >
    > > 1. strings created this way are not freed anywhere else

    >
    > Treat the string object as an object; it will take care of freeing its
    > controlled array.
    >
    > However, your m_stringvector will not natively delete all its strings, so
    > they will leak. Again: Just store, copy, and throw strings, not pointers to
    > string objects, and let them take care of their own low-level details.
    >
    > --
    > Phlip
    > http://www.oreilly.com/catalog/9780596510657/
    > ^ assert_xpath
    > http://tinyurl.com/23tlu5 <-- assert_raise_message



    > Why the new?


    Dont I need to call new to put the strings on the heap? If I just
    declare 2 strings on the stack, won't they die when they go out of
    scope? The whole point was to put them on the heap so that they hang
    around for other threads to process.

    > Make m_stringvector store strings, and just push them in!

    m_stringvector does store strings, and I am pushing strings in (note I
    am pushing back '*s1', not 's1').

    Sorry - I am probably misunderstanding something, or you are talking
    about a concept which is new to me.

    Brad
     
    , Aug 28, 2007
    #4
  5. Phlip Guest

    wrote:

    >> Make m_stringvector store strings, and just push them in!


    > m_stringvector does store strings, and I am pushing strings in (note I
    > am pushing back '*s1', not 's1').


    Didn't notice that, but it's the same difference. Treat objects as objects
    for best results. If you truly need threads (you probably don't!), then put
    entire high-level objects on the heap. All the data within them should
    control themselves. That means the object should store strings, by value.

    > Sorry - I am probably misunderstanding something, or you are talking
    > about a concept which is new to me.


    Try: Resource Acquisition Is Initialization. It means don't abuse the heap,
    and if you do, put the abuse behind clean interfaces that hide the
    controlled objects. For example, that's how a std::string itself works.

    --
    Phlip
    http://www.oreilly.com/catalog/9780596510657/
    ^ assert_xpath
    http://tinyurl.com/23tlu5 <-- assert_raise_message
     
    Phlip, Aug 28, 2007
    #5
  6. Jim Langston Guest

    <> wrote in message
    news:...
    > On Aug 28, 10:46 pm, Phlip <> wrote:
    >> wrote:
    >> > Often in C++ code I find myself having to allocate character arrays to
    >> > pass back to callers for error codes, or as topic/data in message
    >> > passing/observer type scenarios (When having them on the heap is
    >> > necessary to avoid them going out of scope before something else, like
    >> > another thread, has had a chance to process them). What I wanted was a
    >> > neat way to keep track of these allocations within the class, and
    >> > clean them all up nicely, without too much overhead.

    >>
    >> C++ throws objects by copy, so all you need are valid copy semantics.
    >>
    >> > What I hit upon is based on the following ideas:
    >> > 1. std::string is just 'better' than char*, so use it!

    >>
    >> Always follow that rule, for nearly all strings everywhere.
    >>
    >> > 2. std::vector, in its destructor, will attempt to destroy each object
    >> > within it.

    >>
    >> Yes, but destroy does not mean delete.
    >>
    >> > std::string* s1 = new std::string();
    >> > std::string* s2 = new std::string();

    >>
    >> Why the new?
    >>
    >> > //do something with s1 and s2 - i.e. set them and pass them back to a
    >> > function
    >> > //now add them to the class vector to keep track of them:
    >> > m_stringvector.push_back(*s1);
    >> > m_stringvector.push_back(*s2);

    >>
    >> Make m_stringvector store strings, and just push them in!
    >>
    >> > 1. strings created this way are not freed anywhere else

    >>
    >> Treat the string object as an object; it will take care of freeing its
    >> controlled array.
    >>
    >> However, your m_stringvector will not natively delete all its strings, so
    >> they will leak. Again: Just store, copy, and throw strings, not pointers
    >> to
    >> string objects, and let them take care of their own low-level details.


    >> Why the new?

    >
    > Dont I need to call new to put the strings on the heap? If I just
    > declare 2 strings on the stack, won't they die when they go out of
    > scope? The whole point was to put them on the heap so that they hang
    > around for other threads to process.
    >
    >> Make m_stringvector store strings, and just push them in!

    > m_stringvector does store strings, and I am pushing strings in (note I
    > am pushing back '*s1', not 's1').
    >
    > Sorry - I am probably misunderstanding something, or you are talking
    > about a concept which is new to me.


    The std::strings will hang around as long as the vector will. So they won't
    go out of scope until the vector does, when the vecotor delete will cause
    them to be deleted. Things are put into a std::vector by copy. So the
    local std::strings get copied into the vector. The local std::string can go
    out of scope, but the vector's copy of them will remain, until it goes out
    of scope. In other words, no reason for using new. And in this case new is
    a bad thing, because the when the vector's destructor gets called, it will
    call the destructors for the std::string points, not the strings themselves,
    and you'll be leaking memory unless you manually go through and delete the
    elements of the vector.
     
    Jim Langston, Aug 28, 2007
    #6
  7. BobR Guest

    <> wrote in message...
    >
    > Dont I need to call new to put the strings on the heap? If I just
    > declare 2 strings on the stack, won't they die when they go out of
    > scope?


    Not if you 'store' them. They will be destroyed at end-of-scope, but, live
    on in the 'container' (std::vector below).

    #include <iostream>
    #include <string>
    #include <vector>
    #include <iterator>
    #include <algorithm> // copy

    void VecRefFill( std::vector<std::string> &thevec ){
    std::string S1( "Hello" );
    std::string S2( "World" );
    thevec.push_back( S1 );
    thevec.push_back( S2 );
    thevec.push_back( "Third string direct." );
    return;
    } // VecRefFill(vector<string>& )
    // at this point, 'S1' and 'S2' are gone, but,
    // they were copied into the vector.

    std::vector<std::string> VecCopyFill(){
    std::vector<std::string> thevec;
    std::string S1( "Hello2" );
    std::string S2( "World2" );
    thevec.push_back( S1 );
    thevec.push_back( S2 );
    thevec.push_back( "Third string direct2." );
    return thevec; // return a 'copy'
    }

    int main(){
    std::vector<std::string> Vec1;
    VecRefFill( Vec1 );
    std::copy( Vec1.begin(), Vec1.end(),
    std::eek:stream_iterator<std::string>( std::cout, "\n" ) );
    std::cout<<std::endl;

    std::vector<std::string> Vec2;
    Vec2 = VecCopyFill();
    std::copy( Vec2.begin(), Vec2.end(),
    std::eek:stream_iterator<std::string>( std::cout, "\n" ) );

    std::string GetItBack( Vec1.at(2) );
    // std::cout<<GetItBack<<std::endl;

    return 0;
    } // main()
    // at this point, 'Vec1' and 'Vec2' (and all their strings) are gone.
    // all the dynamic memory stuff was handled for you.

    /* -output-
    Hello
    World
    Third string direct.

    Hello2
    World2
    Third string direct2.
    */

    Did that help any?

    --
    Bob R
    POVrookie
    - -
    Get "Thinking in C++", 2nd ed. Volume 1&2 by Bruce Eckel
    (available for free here. You can buy it in hardcopy too.):
    http://www.mindview.net/Books/TICPP/ThinkingInCPP2e.html
    Alf P. Steinbach's "Pointers" document:
    http://home.no.net/dubjai/win32cpptut/special/pointers/ch_01.pdf
    The Dinkumware site has some very good documentation, try:
    http://www.dinkumware.com/manuals/.
    FAQ http://www.parashift.com/c -faq-lite
     
    BobR, Aug 28, 2007
    #7
  8. Guest

    On Aug 29, 3:57 am, "BobR" <> wrote:
    > <> wrote in message...
    >
    > > Dont I need to call new to put the strings on the heap? If I just
    > > declare 2 strings on the stack, won't they die when they go out of
    > > scope?

    >
    > Not if you 'store' them. They will be destroyed at end-of-scope, but, live
    > on in the 'container' (std::vector below).
    >
    > #include <iostream>
    > #include <string>
    > #include <vector>
    > #include <iterator>
    > #include <algorithm> // copy
    >
    > void VecRefFill( std::vector<std::string> &thevec ){
    > std::string S1( "Hello" );
    > std::string S2( "World" );
    > thevec.push_back( S1 );
    > thevec.push_back( S2 );
    > thevec.push_back( "Third string direct." );
    > return;
    > } // VecRefFill(vector<string>& )
    > // at this point, 'S1' and 'S2' are gone, but,
    > // they were copied into the vector.
    >
    > std::vector<std::string> VecCopyFill(){
    > std::vector<std::string> thevec;
    > std::string S1( "Hello2" );
    > std::string S2( "World2" );
    > thevec.push_back( S1 );
    > thevec.push_back( S2 );
    > thevec.push_back( "Third string direct2." );
    > return thevec; // return a 'copy'
    > }
    >
    > int main(){
    > std::vector<std::string> Vec1;
    > VecRefFill( Vec1 );
    > std::copy( Vec1.begin(), Vec1.end(),
    > std::eek:stream_iterator<std::string>( std::cout, "\n" ) );
    > std::cout<<std::endl;
    >
    > std::vector<std::string> Vec2;
    > Vec2 = VecCopyFill();
    > std::copy( Vec2.begin(), Vec2.end(),
    > std::eek:stream_iterator<std::string>( std::cout, "\n" ) );
    >
    > std::string GetItBack( Vec1.at(2) );
    > // std::cout<<GetItBack<<std::endl;
    >
    > return 0;
    > } // main()
    > // at this point, 'Vec1' and 'Vec2' (and all their strings) are gone.
    > // all the dynamic memory stuff was handled for you.
    >
    > /* -output-
    > Hello
    > World
    > Third string direct.
    >
    > Hello2
    > World2
    > Third string direct2.
    > */
    >
    > Did that help any?
    >
    > --
    > Bob R
    > POVrookie
    > - -
    > Get "Thinking in C++", 2nd ed. Volume 1&2 by Bruce Eckel
    > (available for free here. You can buy it in hardcopy too.):
    > http://www.mindview.net/Books/TICPP/ThinkingInCPP2e.html
    > Alf P. Steinbach's "Pointers" document:http://home.no.net/dubjai/win32cpptut/special/pointers/ch_01.pdf
    > The Dinkumware site has some very good documentation, try:
    > http://www.dinkumware.com/manuals/.
    > FAQ http://www.parashift.com/c -faq-lite


    Yes - that has helped. I did not know that local objects added to a
    vector would live on, which is quite convenient. So I have learned
    something from your excellent answers (all posters), which was the
    whole point of posting this in the first place.

    The only issue now (and possibly a reason to use my implementation
    over the ones suggested) is that the API's that I am passing back to
    need to take a char*.

    std::string s1 = "Hello, World";
    call_some_function(s1.c_str()); //Pass a const char* to some api
    m_stringvector.push_back(s1);

    Now, once s1 goes out of scope, it will destruct. I assume the copy of
    s1 sitting in the vector is exactly that - a copy. So the pointer
    value of the vectors s1.c_str() would be different to the local
    s1.c_str() (even though they would both contain the same string data).
    This means that once s1 goes out of scope, my c_str() array pointers
    that I have passed to external API's are invalid. Unless I do:

    std::string s1 = "Hello, World";
    m_stringvector.push_back(s1);
    call_some_function((m_stringvector.back()).c_str());

    In any case, I guess I just decided to put them on the heap and make
    sure objects (and not pointers) were put in the vectors. To be clear,
    in case people have missed it:

    std::vector<std::string> m_stringvector; //this is private a class
    member of SomeClass

    ....in SomeClass::someMethod()...
    std::string* s1 = new std::string();
    *s1 = "Hello, World";
    call_some_function(s1->c_str());
    m_stringvector.push_back(*s1); //Note I am pushing *s1 here, which is
    a std::string, not a pointer.


    So can I just confirm that there is no memory leak here, and that the
    vector will clean up the string object(s) when the class destructs? I
    have seen a few people saying that this will leak, but I am not sure
    if it is because they have missed the fact that I am actually putting
    string objects in the vector (and not pointers), or if there actually
    is a memory problem here.

    I can see from the suggestions that I don't really need to use new()
    here, and I will probably change the implementation, but I just want
    to make sure that my existing implementation is not fundamentally
    flawed.

    Brad

    P.S. I tried to avoid threads, but the other api's I am working with
    need me to use them, or hang the UI while I do some long task.
     
    , Aug 29, 2007
    #8
  9. Phlip Guest

    brad.power wrote:

    > std::string s1 = "Hello, World";
    > call_some_function(s1.c_str()); //Pass a const char* to some api
    > m_stringvector.push_back(s1);
    >
    > Now, once s1 goes out of scope, it will destruct. I assume the copy of
    > s1 sitting in the vector is exactly that - a copy. So the pointer
    > value of the vectors s1.c_str() would be different to the local
    > s1.c_str() (even though they would both contain the same string data).
    > This means that once s1 goes out of scope, my c_str() array pointers
    > that I have passed to external API's are invalid.


    Can you positively determine this? Can you read the API's source, or its
    documentation, or its tests?

    Can you do this?

    char yo[] = "hello world";
    call_some_function(yo);
    strcpy(yo, "bye-bye");

    What string comes out the other end of this API? I don't know about other
    programmers, but if I were writing a C API then I would take the Interface
    part very seriously, and would not abuse pointers to things my clients
    passed in.

    > P.S. I tried to avoid threads, but the other api's I am working with
    > need me to use them, or hang the UI while I do some long task.


    In this case, you need to learn threading, and then use a semaphore to
    ensure your thread-side objects live as long as the thread. And you need to
    put your strings into a container (like your vector) which is itself a
    member of these thread-side objects. Put the entire object onto the side of
    your API - not just each danged string by itself.

    And, are you absolutely certain your GUI will hang if you run the API? Does
    it have any timer or semaphore system you can use to strobe your GUI?
    Sometimes these systems are more robust...

    --
    Phlip
    http://www.oreilly.com/catalog/9780596510657/
    "Test Driven Ajax (on Rails)"
    assert_xpath, assert_javascript, & assert_ajax
     
    Phlip, Aug 29, 2007
    #9
  10. Guest

    On Aug 29, 11:08 am, "Phlip" <> wrote:
    > brad.power wrote:
    > > std::string s1 = "Hello, World";
    > > call_some_function(s1.c_str()); //Pass a const char* to some api
    > > m_stringvector.push_back(s1);

    >
    > > Now, once s1 goes out of scope, it will destruct. I assume the copy of
    > > s1 sitting in the vector is exactly that - a copy. So the pointer
    > > value of the vectors s1.c_str() would be different to the local
    > > s1.c_str() (even though they would both contain the same string data).
    > > This means that once s1 goes out of scope, my c_str() array pointers
    > > that I have passed to external API's are invalid.

    >
    > Can you positively determine this? Can you read the API's source, or its
    > documentation, or its tests?
    >
    > Can you do this?
    >
    > char yo[] = "hello world";
    > call_some_function(yo);
    > strcpy(yo, "bye-bye");
    >
    > What string comes out the other end of this API? I don't know about other
    > programmers, but if I were writing a C API then I would take the Interface
    > part very seriously, and would not abuse pointers to things my clients
    > passed in.


    So what you are asking is whether or not the other side of the API
    takes a copy of the string or just uses whatever the passed pointer
    points to? I have done a test and found that it doesn't make its own
    copy - it just passes the pointer around internally. Thats actually
    the problem I am trying to solve - I am making long-lived copies of
    the data in my code so that the pointers passed will continue to be
    valid, and I just wanted to see if my 'use a vector to keep track and
    clean up' idea was a good one.

    >
    > > P.S. I tried to avoid threads, but the other api's I am working with
    > > need me to use them, or hang the UI while I do some long task.

    >
    > In this case, you need to learn threading, and then use a semaphore to
    > ensure your thread-side objects live as long as the thread. And you need to
    > put your strings into a container (like your vector) which is itself a
    > member of these thread-side objects. Put the entire object onto the side of
    > your API - not just each danged string by itself.
    >


    That is exactly what I am doing. This container is sitting in a thread-
    side object which is guaranteed to have the same lifetime as the
    thread. The vector is meant to be an automatic clean up system, since
    once I kick the thread off I want it to clean up after itself.

    > And, are you absolutely certain your GUI will hang if you run the API? Does
    > it have any timer or semaphore system you can use to strobe your GUI?
    > Sometimes these systems are more robust...


    Yep. I tried it without threads and it hung, and then I went to the
    documentation, which told me to use threads. :)

    >
    > --
    > Phlip
    > http://www.oreilly.com/catalog/9780596510657/
    > "Test Driven Ajax (on Rails)"
    > assert_xpath, assert_javascript, & assert_ajax


    Thanks for your help.
     
    , Aug 29, 2007
    #10
  11. BobR Guest

    <> wrote in message...
    > [snip]
    > The only issue now (and possibly a reason to use my implementation
    > over the ones suggested) is that the API's that I am passing back to
    > need to take a char*.
    >
    > std::string s1 = "Hello, World";
    > call_some_function(s1.c_str()); file://Pass a const char* to some api
    > m_stringvector.push_back(s1);
    >
    > Now, once s1 goes out of scope, it will destruct. I assume the copy of
    > s1 sitting in the vector is exactly that - a copy. So the pointer
    > value of the vectors s1.c_str() would be different to the local
    > s1.c_str() (even though they would both contain the same string data).
    > This means that once s1 goes out of scope, my c_str() array pointers
    > that I have passed to external API's are invalid. Unless I do:
    >
    > std::string s1 = "Hello, World";
    > m_stringvector.push_back(s1);
    > call_some_function((m_stringvector.back()).c_str());
    >
    > In any case, I guess I just decided to put them on the heap and make
    > sure objects (and not pointers) were put in the vectors. To be clear,
    > in case people have missed it:
    >
    > std::vector<std::string> m_stringvector; file://this is private a class
    > member of SomeClass
    >
    > ...in SomeClass::someMethod()...
    > std::string* s1 = new std::string();
    > *s1 = "Hello, World";

    // std::string *s1 = new std::string( "Hello, World" );

    > call_some_function(s1->c_str());
    > m_stringvector.push_back(*s1); file://Note I am pushing *s1 here, which is
    > a std::string, not a pointer.
    >
    > So can I just confirm that there is no memory leak here, and that the
    > vector will clean up the string object(s) when the class destructs? I
    > have seen a few people saying that this will leak, but I am not sure
    > if it is because they have missed the fact that I am actually putting
    > string objects in the vector (and not pointers), or if there actually
    > is a memory problem here.


    You pushed a copy of the string into the vector. The vector will destruct
    those. But, what about 's1'? You 'new' it, you 'delete' it.

    [ something to look out for ]
    If you put pointers into a vector, it will destruct the pointers when the
    vector is destructed, BUT, not what they point to. You need to do that
    before a vector gets destroyed.

    { // some scope
    std::vector<Thing*> VecThing; // assume 'class Thing'.
    VecThing.push_back( new Thing );
    VecThing.push_back( new Thing );

    // use VecThing here

    for( std::size_t i(0); i < VecThing.size(); ++i ){
    delete VecThing.at(i); // delete the Things
    } // for(i)
    } // the Thing pointers are destructed at this point.


    --
    Bob R
    POVrookie
     
    BobR, Aug 29, 2007
    #11
  12. Guest

    On Aug 29, 1:17 pm, "BobR" <> wrote:
    > <> wrote in message...
    > > [snip]
    > > The only issue now (and possibly a reason to use my implementation
    > > over the ones suggested) is that the API's that I am passing back to
    > > need to take a char*.

    >
    > > std::string s1 = "Hello, World";
    > > call_some_function(s1.c_str()); file://Pass a const char* to some api
    > > m_stringvector.push_back(s1);

    >
    > > Now, once s1 goes out of scope, it will destruct. I assume the copy of
    > > s1 sitting in the vector is exactly that - a copy. So the pointer
    > > value of the vectors s1.c_str() would be different to the local
    > > s1.c_str() (even though they would both contain the same string data).
    > > This means that once s1 goes out of scope, my c_str() array pointers
    > > that I have passed to external API's are invalid. Unless I do:

    >
    > > std::string s1 = "Hello, World";
    > > m_stringvector.push_back(s1);
    > > call_some_function((m_stringvector.back()).c_str());

    >
    > > In any case, I guess I just decided to put them on the heap and make
    > > sure objects (and not pointers) were put in the vectors. To be clear,
    > > in case people have missed it:

    >
    > > std::vector<std::string> m_stringvector; file://this is private a class
    > > member of SomeClass

    >
    > > ...in SomeClass::someMethod()...
    > > std::string* s1 = new std::string();
    > > *s1 = "Hello, World";

    >
    > // std::string *s1 = new std::string( "Hello, World" );
    >
    > > call_some_function(s1->c_str());
    > > m_stringvector.push_back(*s1); file://Note I am pushing *s1 here, which is
    > > a std::string, not a pointer.

    >
    > > So can I just confirm that there is no memory leak here, and that the
    > > vector will clean up the string object(s) when the class destructs? I
    > > have seen a few people saying that this will leak, but I am not sure
    > > if it is because they have missed the fact that I am actually putting
    > > string objects in the vector (and not pointers), or if there actually
    > > is a memory problem here.

    >
    > You pushed a copy of the string into the vector. The vector will destruct
    > those. But, what about 's1'? You 'new' it, you 'delete' it.


    Ok so this might be a problem. If I new something, and then push the
    object (dereferenced pointer) into the vector, does it copy the
    object? If it does then I have a leak, and it would appear that
    pushing local (non new()ed) strings into the vector is a better
    approach.

    >
    > [ something to look out for ]
    > If you put pointers into a vector, it will destruct the pointers when the
    > vector is destructed, BUT, not what they point to. You need to do that
    > before a vector gets destroyed.


    Yep I am aware of this, but I am not pushing pointers onto the vector
    (it is a vector of string, not a vector of string*). I am pushing what
    the pointers point to.

    m_stringvector.push_back(*s1);

    >
    > { // some scope
    > std::vector<Thing*> VecThing; // assume 'class Thing'.
    > VecThing.push_back( new Thing );
    > VecThing.push_back( new Thing );
    >
    > // use VecThing here
    >
    > for( std::size_t i(0); i < VecThing.size(); ++i ){
    > delete VecThing.at(i); // delete the Things
    > } // for(i)
    >
    > } // the Thing pointers are destructed at this point.
    >
    > --
    > Bob R
    > POVrookie


    Right. So if when you push_back an object into a vector, a *copy* of
    that object is made (in seperate storage), then I have a memory leak,
    since nothing deletes the initial new(). If this is the case, then it
    would be better to push locally declared strings into the vector. I
    can test this with a quick test harness.

    Thanks all for your comments, I have learned a lot.
    Brad
     
    , Aug 29, 2007
    #12
  13. On 29 Aug, 03:22, "" <> wrote:
    >
    > std::string s1 = "Hello, World";
    > m_stringvector.push_back(s1);
    > call_some_function((m_stringvector.back()).c_str());
    >


    There's potentially a subtle ticking time bomb here -
    if the call_some_function code caches the const char*
    you're passing, all will be well *until* your m_stringvector
    gets resized (for example by you pushing more strings).
    If that happens then its contained strings will all get
    moved, which (depending on your std::string implementation)
    is going to invalidate that const char* pointer.

    In this situation, if you can't predict how many elements
    m_stringvector will store, then there are a couple of
    alternatives that I can see;
    a) use a deque instead of a vector; provided you only
    insert at the back (or the front) you won't get an
    element-invalidating resize
    b) use std::string* instead !

    >
    > ...in SomeClass::someMethod()...
    > std::string* s1 = new std::string();
    > *s1 = "Hello, World";
    > call_some_function(s1->c_str());
    > m_stringvector.push_back(*s1); //Note I am pushing *s1 here, which is
    > a std::string, not a pointer.
    >
    > So can I just confirm that there is no memory leak here, and that the
    > vector will clean up the string object(s) when the class destructs? I
    > have seen a few people saying that this will leak, but I am not sure
    > if it is because they have missed the fact that I am actually putting
    > string objects in the vector (and not pointers), or if there actually
    > is a memory problem here.
    >


    You do still have a leak here; s1 leaks. m_stringvector will indeed
    clear up its contained strings, but you're storing a copy of *s1; s1
    itself is still your problem.

    HTH
     
    tragomaskhalos, Aug 29, 2007
    #13
  14. Guest

    On Aug 29, 9:29 pm, tragomaskhalos <>
    wrote:
    > On 29 Aug, 03:22, "" <> wrote:
    >
    >
    >
    > > std::string s1 = "Hello, World";
    > > m_stringvector.push_back(s1);
    > > call_some_function((m_stringvector.back()).c_str());

    >
    > There's potentially a subtle ticking time bomb here -
    > if the call_some_function code caches the const char*
    > you're passing, all will be well *until* your m_stringvector
    > gets resized (for example by you pushing more strings).
    > If that happens then its contained strings will all get
    > moved, which (depending on your std::string implementation)
    > is going to invalidate that const char* pointer.
    >
    > In this situation, if you can't predict how many elements
    > m_stringvector will store, then there are a couple of
    > alternatives that I can see;
    > a) use a deque instead of a vector; provided you only
    > insert at the back (or the front) you won't get an
    > element-invalidating resize
    > b) use std::string* instead !


    Yeah sorry using m_stringvector.back() was just for the purposes of
    the example, to show that I had added the string to the vector, and
    *then* used the vectored string in the external call, instead of the
    other way around. I wouldn't actually do that exact code sequence in
    real code. Thanks for pointing it out though - you are quite correct.

    >
    >
    >
    > > ...in SomeClass::someMethod()...
    > > std::string* s1 = new std::string();
    > > *s1 = "Hello, World";
    > > call_some_function(s1->c_str());
    > > m_stringvector.push_back(*s1); //Note I am pushing *s1 here, which is
    > > a std::string, not a pointer.

    >
    > > So can I just confirm that there is no memory leak here, and that the
    > > vector will clean up the string object(s) when the class destructs? I
    > > have seen a few people saying that this will leak, but I am not sure
    > > if it is because they have missed the fact that I am actually putting
    > > string objects in the vector (and not pointers), or if there actually
    > > is a memory problem here.

    >
    > You do still have a leak here; s1 leaks. m_stringvector will indeed
    > clear up its contained strings, but you're storing a copy of *s1; s1
    > itself is still your problem.
    >


    Yep you are right - I verified this myself with a few tests. Looks
    like pushing stack-scoped strings onto the vector is the way to go.

    > HTH


    Yes it has helped - I would like to reiterate that everyone's
    contribution to the thread has helped me improve the implementation of
    this little cleanup idea, so thankyou to everyone who replied.

    Brad
     
    , Aug 29, 2007
    #14
  15. BobR Guest

    <> wrote in message...
    > On Aug 29, 1:17 pm, "BobR" <> wrote:
    > > You pushed a copy of the string into the vector. The vector will

    destruct
    > > those. But, what about 's1'? You 'new' it, you 'delete' it.

    >
    > Ok so this might be a problem. If I new something, and then push the
    > object (dereferenced pointer) into the vector, does it copy the
    > object? If it does then I have a leak, and it would appear that
    > pushing local (non new()ed) strings into the vector is a better
    > approach.
    >


    Rather than re-post a bunch of stuff, you may want to check out this thread:

    ----- Original Message -----
    From: BobR
    Newsgroups: comp.lang.c++
    Sent: Saturday, June 09, 2007 10:43 PM
    Subject: Re: problems of storing dynamically created objects in a vector


    > >
    > > [ something to look out for ]
    > > If you put pointers into a vector, it will destruct the pointers when

    the
    > > vector is destructed, BUT, not what they point to. You need to do that
    > > before a vector gets destroyed.

    >
    > Yep I am aware of this, but I am not pushing pointers onto the vector
    > (it is a vector of string, not a vector of string*). I am pushing what
    > the pointers point to.
    > m_stringvector.push_back(*s1);
    > [snip]
    >
    > Right. So if when you push_back an object into a vector, a *copy* of
    > that object is made (in seperate storage), then I have a memory leak,
    > since nothing deletes the initial new(). If this is the case, then it
    > would be better to push locally declared strings into the vector. I
    > can test this with a quick test harness.


    It looks like you are depending on a pointer to the string not changing
    addresses, so, push_back is not good. You need to know how many strings will
    be in use and set your vector to an initial size.

    std::vector<std::string> vStr( 100, std::string( "Hi" ) );
    // now you have 100 strings, all set to "Hi", in the vector.
    // or: std::vector<std::string> vStr( 100 ); // 100 empty strings.
    vStr.at( 2 ) = "Hello World"; // ok
    vStr.push_back( "Goodbye" ); // causes re-allocation
    vStr.resize(150); // causes re-allocation
    // you can check with 'vStr.capacity()'
    // if size exceeds capacity, it re-allocates,
    // (possibly/usually) changing it's address.

    So, I suggest you dump the 'new', and manage the lifetime and size of the
    vector.
    Let the std library do the dirty work for you. <G>

    --
    Bob R
    POVrookie
     
    BobR, Aug 29, 2007
    #15
    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. Weng Tianxiang
    Replies:
    4
    Views:
    3,148
    Weng Tianxiang
    Apr 7, 2005
  2. Dave Swersky

    ASP.NET Technique Headcheck

    Dave Swersky, Aug 13, 2003, in forum: ASP .Net
    Replies:
    0
    Views:
    343
    Dave Swersky
    Aug 13, 2003
  3. Jim Heavey

    Best Technique

    Jim Heavey, Nov 11, 2003, in forum: ASP .Net
    Replies:
    0
    Views:
    367
    Jim Heavey
    Nov 11, 2003
  4. Patrick Kristiansen

    Templating technique

    Patrick Kristiansen, Jan 24, 2004, in forum: ASP .Net
    Replies:
    1
    Views:
    355
    Natty Gur
    Jan 26, 2004
  5. jqpdev
    Replies:
    5
    Views:
    525
    jqpdev
    Feb 28, 2004
Loading...

Share This Page