20

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

1
  • Try using at() for accessing it won't let you access something that is not there example: ideone.com/ZeRTnd (though item with key "B" was defined before, after map.clear() trying to accessing key "B" gets you 'std::out_of_range' exception
    – jave.web
    Commented Aug 30, 2015 at 14:33

6 Answers 6

23

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.

1
  • 7
    It only menas that you may be accessing garbage, but sometimes you access an area that hasn't become garbage yet, so it seems to work for a while. Which is why "but it works, so I can get away with it" is totally false. Commented Mar 27, 2011 at 12:23
19

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 )

;

0
4

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.

6
  • So if I don't use any sort of smart pointer classes, do I need go through the map and manually delete every single element? How do I clear the map after that? I mean, clear() will attempt to call destructors of elements that were already deleted.
    – sneg
    Commented Feb 20, 2009 at 11:02
  • 1
    Just delete all of your pointers, and then call clear(). As jpalecek told you, calling the destructor of a pointer (which is totally different from calling the desructor of the object it is pointing to) does nothing.
    – Marc
    Commented Feb 20, 2009 at 11:06
  • Just to clarify: if you store actual objects in a map, its destructor will be called. But if you store pointers (any kind of pointer), the map will only know that it has a bunch of meaningless pointers, and will treat them as what they essentially are: integers representing memory addresses.
    – Marc
    Commented Feb 20, 2009 at 11:09
  • Thanks, Marc. I understand now. In my code snippet above destructor of struct A does get called. I.e. element x is set to 0. I was worried that if I delete this instance of struct A map.clear() will attempt to call destructor again.
    – sneg
    Commented Feb 20, 2009 at 11:17
  • 1
    sneg-vx: This is strange. I tried it with gcc 4.3 and got 5 two times in the output, so the destructor wasn't called (as the standard says).
    – jpalecek
    Commented Feb 20, 2009 at 17:20
1

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.

0

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.

0

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.

2
  • 1
    While a bit of a trade-off, having the Map allocate it's own data instead of a pointer to hold the return from your malloc() can also be more memory efficient, since your malloc() and the Map's malloc required to hold your data will be the same size, saving the memory requirement for the now redundant pointer. 64-bit pointers add up to a lot of storage. This is traded off against inner nodes of the Map/Tree allocating space for storing things they'll never store - although it keeps the heap from fragmenting since all malloc()s are the same size when nodes are added or deleted.
    – user1899861
    Commented Aug 19, 2013 at 6:14
  • One you might add, is to use malloc() instead of new(), as it makes no sense to allocate and memset() memory with new() when you're in the process of memcpy()ing data into that new storage. It's a total and complete waste of whatever time it takes to initialize the storage. If you want to do this for debugging purposes, use calloc(). The STL uses malloc() and free() internally, so take that cue and run with it.
    – user1899861
    Commented Sep 10, 2013 at 0:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.