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

[cboard_member]

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 );
        }
        else
        {
            (*it).second->strField = new char[strlen(str)];
            sprintf( (*it).second->strField, "%s", str );
        }
    }

    return true;
}

[VirtualAce]

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;
  protected:
    CBitmap *m_pBitmap;
    CBitmap *m_pMask;
    CDC        *m_pDC;
    DWORD   m_dwID;
    
  private:
    CTile():m_pBitmap(NULL),m_pMask(NULL),m_pDC(NULL),m_dwID(0) { }
    virtual ~CTile()
    {
       SAFE_DELETE(m_pBitmap);
       SAFE_DELETE(m_pMask);
       m_pDC->DeleteDC();
       SAFE_DELETE(m_pDC);
    }
    
    void Create();
    ...
};

class CTileMgr
{
  protected:
    std::map<CTile *> m_mapTiles;
  public:
    CTileMGr();
    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.

[CornedBee]

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.