Thread: delete[]'ing std::map elements... sort of.

  1. #1
    Supermassive black hole cboard_member's Avatar
    Join Date
    Jul 2005

    delete[]'ing std::map elements... sort of.

    Okey dokey, I've tried this a million times before and just give up because I can't figure it out. Basically in my Cvar manager class I'm using a std::map, mapping names to Cvar*'s. Everything else works fine, I just want to free some memory. Each Cvar has an strField, which may or may not be allocated depending on what the caller wants to store in it. The Cvar itself is also allocated.

    Now, this cleanup routine seems to work (it didn't crash at least):

    void CvarMgr::Cleanup()
        CvarMapType::iterator   it = _cvars.begin();
        CvarMapType::iterator   ite = _cvars.end();
        for ( ; it != ite; it++ )
            if ( (*it).second->strField )
                delete[] (*it).second->strField;
            delete (*it).second;
    But, with my Set routine, which modifies the values of an existing Cvar, I can't delete[] strField without one of those "damaged block" runtime dialogs.

    /*  Set
     *  Sets the values of an existing variable. This may be a little
     *  awkward to call. Should probably revise it.
    bool CvarMgr::Set( const std::string& name, ULONG ul, long l, float f, double d, BYTE by, bool b, const char* str )
        CvarMapType::iterator   it;
        if ( (it = _cvars.find(name)) == _cvars.end() )
            ErrorLogging::Log( Va("Cvar '%s' doesn't exist (trying to set it)", name.c_str()), true );
            return false;
        (*it).second->bField = b;
        (*it).second->byField = by;
        (*it).second->dField = d;
        (*it).second->fField = f;
        (*it).second->lField = l;
        (*it).second->ulField = ul;
        if ( str )
            if ( (*it).second->strField )
                delete[] (*it).second->strField; // Dies right here 
                (*it).second->strField = new char[strlen(str)];
                sprintf( (*it).second->strField, "%s", str );
                (*it).second->strField = new char[strlen(str)];
                sprintf( (*it).second->strField, "%s", str );
        return true;
    Good class architecture is not like a Swiss Army Knife; it should be more like a well balanced throwing knife.

    - Mike McShaffry

  2. #2
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    To clean your code up you will want to create a class that encapsulates one CVar object.

    I'll demonstrate with a MFC-based Win32 tile object along with associated tile mgr.

    class CTileMgr;
    class CTile
      friend class CTileMgr;
        CBitmap *m_pBitmap;
        CBitmap *m_pMask;
        CDC        *m_pDC;
        DWORD   m_dwID;
        CTile():m_pBitmap(NULL),m_pMask(NULL),m_pDC(NULL),m_dwID(0) { }
        virtual ~CTile()
        void Create();
    class CTileMgr
        std::map<CTile *> m_mapTiles;
        virtual ~CTileMgr()
           for (DWORD dwTile=0;dwTile<static_cast<DWORD>(m_mapTiles.size());dwTile++)
              delete m_mapTiles[i];
    The delete[] inside of the CTileMgr destructor offloads the bulk of the work to the object. In other words, CTile knows how to clean itself up and since CTileMgr manages a map or collection of CTile objects, as long as CTile cleans up perfectly, CTileMgr will do so as well.

    You have the manager class doing too much. And you are trying to delete elements of the object rather than the object itself. Items that need to be cleaned up should not be cleaned up directly by the collection class, but by the class that handles the object in the collection. Your manager or collection class then becomes a simple wrapper for accessing the objects it is acting as a manager for.

    If this is managing a collection of variable name to variable value objects, you would still call delete on the entire object rather than attempting to remove the items the way you are. Also if you are attempting to alter data items inside of the CVar object from within the CVarMgr or collection class, IMO, this is bad practice. It would be much better to create a Set() function in the object class or CVar class and then call Set from the CVarMgr::Set function. This is 2 function calls to do one thing, but it does offload the work to the object.

    I'll give you the source to CTileGDI and CTileMgrGDI to serve as an example.
    Last edited by VirtualAce; 03-12-2011 at 10:42 AM.

  3. #3
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    1) Iterators overload ->. Use it.
    2) std::string manages its own memory. Use it.
    3) boost::shared_ptr automatically deletes its pointee when the last pointer is destructed. Use it.
    All the buzzt!

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Sort two vectors in the same order
    By jw232 in forum C++ Programming
    Replies: 2
    Last Post: 03-08-2008, 04:49 PM
  2. threaded merge sort
    By AusTex in forum Linux Programming
    Replies: 4
    Last Post: 05-04-2005, 04:03 AM
  3. recursive quick sort - stack overflow
    By Micko in forum C Programming
    Replies: 9
    Last Post: 01-01-2005, 04:51 PM
  4. Help On A Quick Sort Program
    By nick4 in forum C++ Programming
    Replies: 11
    Last Post: 12-06-2004, 09:51 AM
  5. Replies: 10
    Last Post: 09-15-2004, 01:00 PM
Website Security Test