Why is memory still accessible after std::map::clear() is called?

Question

I am observing strange behaviour of std::map::clear(). This method is supposed to call element's destructor when called, however memory is still accessible after call to clear().

For example:

struct A
{
  ~A() { x = 0; }
  int x;
};

int main( void )
{
  std::map< int, A * > my_map;
  A *a = new A();
  a->x = 5;
  my_map.insert( std::make_pair< int, *A >( 0, a ) );

  // addresses will be the same, will print 5
  std::cout << a << " " << my_map[0] << " " << my_map[0]->x << std::endl;

  my_map.clear();

  // will be 0
  std::cout << a->x << std::endl;

  return 0;
}

The question is, why is variable a still accessible after its destructor was called by map::clear()? Do I need to write delete a; after calling my_map.clear() or is it safe to overwrite the contents of a?

Thanks in advance for your help, sneg

Answer 1

std::map does not manage the memory pointed to by the pointer values - it's up to you to do it yourself. If you don't want to use smart pointers, you can write a general purpose free & clear function like this:

template <typename M> void FreeClear( M & amap ) 
    for ( typename M::iterator it = amap.begin(); it != amap.end(); ++it ) {
        delete it->second;
    }
    amap.clear();
}

And use it:

std::map< int, A * > my_map;
// populate
FreeClear( my_map )

;

Answer 2

If you store pointers on a map (or a list, or anything like that) YOU are the responsible for deleting the pointers, since the map doesn't know if they have been created with new, or not. The clear function only invokes destructors if you don't use pointers.

Oh, and one more thing: invoking a destructor (or even calling delete) doesn't mean the memory can't be accessed anymore. It only means that you will be accessing garbage if you do.

Answer 3

That's because map.clear() calls destructors of the data contained in the map, in your case, of the pointer to a. And this does nothing.

You might want to put some kind of smart pointer in the map for the memory occupied by a to be automatically reclaimed.

BTW, why do you put the template arguments in the call to make_pair? The template argument deduction should do pretty well here.

Answer 4

When you free a piece of heap memory, its contents don't get zeroed. They are merely available for allocation again. Of course you should consider the memory non accessible, because the effects of accessing unallocated memory are undefined.

Actually preventing access to a memory page happens on a lower level, and std libraries don't do that.

When you allocate memory with new, you need to delete it yourself, unless you use a smart pointer.

Answer 5

Any container stores your object Type and call corresponding constructors: internal code each node might look similar to:

__NodePtr
{
    *next;
    __Ty    Val;
}

When you allocate it happens by constructing the val based on type and then linking. Something similar to:

_Ty _Val = _Ty();
_Myhead = _Buynode();
_Construct_n(_Count, _Val);

When you delete it calls corresponding destructors.

When you store references (pointers) it won't call any constructor nor it will destruct.

Answer 6

Having spent the last 2 months eating, sleeping, and breathing maps, I have a recommendation. Let the map allocate it's own data whenever possible. It's a lot cleaner, for exactly the kind of reasons you're highlighting here.

There are also some subtle advantages, like if you're copying data from a file or socket to the map's data, the data storage exists as soon as the node exists because when the map calls malloc() to allocate the node, it allocates memory for both the key and the data. (AKA map[key].first and map[key].second)

This allows you to use the assignment operator instead of memcpy(), and requires 1 less call to malloc() - the one you make.

IC_CDR CDR, *pThisCDRLeafData;  // a large struct{}

    while(1 == fread(CDR, sizeof(CDR), 1, fp))  {
    if(feof(fp)) {
        printf("\nfread() failure in %s at line %i", __FILE__, __LINE__);
    }
    cdrMap[CDR.iGUID] = CDR; // no need for a malloc() and memcpy() here    
    pThisCDRLeafData  = &cdrMap[CDR.iGUID]; // pointer to tree node's data

A few caveats to be aware of are worth pointing out here.

1. do NOT call malloc() or new in the line of code that adds the tree node as your call to malloc() will return a pointer BEFORE the map's call to malloc() has allocated a place to hold the return from your malloc().
2. in Debug mode, expect to have similar problems when trying to free() your memory. Both of these seem like compiler problems to me, but at least in MSVC 2012, they exist and are a serious problem.

3. give some thought as to where to "anchor" your maps. IE: where they are declared. You don't want them going out of scope by mistake. main{} is always safe.

   INT _tmain(INT argc, char* argv[])    {
   IC_CDR      CDR, *pThisCDRLeafData=NULL;
   CDR_MAP     cdrMap;
   CUST_MAP    custMap;
   KCI_MAP     kciMap;
   

4. I've had very good luck, and am very happy having a critical map allocate a structure as it's node data, and having that struct "anchor" a map. While anonymous structs have been abandoned by C++ (a horrible, horrible decision that MUST be reversed), maps that are the 1st struct member work just like anonymous structs. Very slick and clean with zero size-effects. Passing a pointer to the leaf-owned struct, or a copy of the struct by value in a function call, both work very nicely. Highly recommended.

5. you can trap the return values for .insert to determine if it found an existing node on that key, or created a new one. (see #12 for code) Using the subscript notation doesn't allow this. It might be better to settle on .insert and stick with it, especially because the [] notation doesn't work with multimaps. (it would make no sense to do so, as there isn't "a" key, but a series of keys with the same values in a multimap)
6. you can, and should, also trap returns for .erase and .empty() (YES, it's annoying that some of these things are functions, and need the () and some, like .erase, don't)
7. you can get both the key value and the data value for any map node using .first and .second, which all maps, by convention, use to return the key and data respectively

8. save yourself a HUGE amount of confusion and typing, and use typedefs for your maps, like so.

   typedef map<ULLNG, IC_CDR>      CDR_MAP;    
   typedef map<ULLNG, pIC_CDR>     CALL_MAP;   
   typedef struct {
       CALL_MAP    callMap;
       ULNG        Knt;         
       DBL         BurnRateSec; 
       DBL         DeciCents;   
       ULLNG       tThen;       
       DBL         OldKCIKey;   
   } CUST_SUM, *pCUST_SUM;
   typedef map<ULNG,CUST_SUM>  CUST_MAP, CUST_MAP;  
   typedef map<DBL,pCUST_SUM>  KCI_MAP;
   

9. pass references to maps using the typedef and & operator as in

   ULNG DestroyCustomer_callMap(CUST_SUM Summary, CDR_MAP& cdrMap, KCI_MAP& kciMap)

10. use the "auto" variable type for iterators. The compiler will figure out from the type specified in the rest of the for() loop body what kind of map typedef to use. It's so clean it's almost magic!

    for(auto itr = Summary.callMap.begin(); itr!= Summary.callMap.end(); ++itr) {

11. define some manifest constants to make the return from .erase and .empty() more meaningfull.

    if(ERASE_SUCCESSFUL == cdrMap.erase (itr->second->iGUID)) {

12. given that "smart pointers" are really just keeping a reference count, remember you can always keep your own reference count, an probably in a cleaner, and more obvious way. Combining this with #5 and #10 above, you can write some nice clean code like this.

    #define Pear(x,y) std::make_pair(x,y) //  some macro magic
    
    auto res = pSumStruct->callMap.insert(Pear(pCDR->iGUID,pCDR));
    if ( ! res.second ) {
        pCDR->RefKnt=2;
    } else {
        pCDR->RefKnt=1;
        pSumStruct->Knt += 1;
    }
    

13. using a pointer to hang onto a map node which allocates everything for itself, IE: no user pointers pointing to user malloc()ed objects, works well, is potentially more efficient, and and be used to mutate a node's data without side-effects in my experience.

14. on the same theme, such a pointer can be used very effectively to preserve the state of a node, as in pThisCDRLeafData above. Passing this to a function that mutates/changes that particular node's data is cleaner than passing a reference to the map and the key needed to get back to the node pThisCDRLeafData is pointing to.

15. iterators are not magic. They are expensive and slow, as you are navigating the map to get values. For a map holding a million values, you can read a node based on a key at about 20 million per second. With iterators it's probably ~ 1000 times as slow.

I think that about covers it for now. Will update if any of this changes or there's additional insights to share. I am especially enjoying using the STL with C code. IE: not a class in sight anywhere. They just don't make sense in the context I'm working in, and it's not an issue. Good luck.