memory errors with std::map

[rilpo]

Hello everyone,
I have some troubles with memory handling of std::map.
Here is an example of code.

#include <map>
#include <iostream>

struct MonDouble{
    MonDouble(double d): v(d) {}
    ~MonDouble(){}
    double v;
};

int main(int /*argc*/, char **/*argv*/) {

    std::map<std::string,MonDouble*> mymap;
    std::map<std::string,MonDouble*>::iterator it;

    //insert one value (Part 1)
    it = mymap.find("VAL");
    if(it != mymap.end()){
        delete it->second;
    }
    mymap.insert(std::make_pair("VAL",new MonDouble(1.5)));

    //modify the value (Part 2)
    it = mymap.find("VAL");
    if(it != mymap.end()){
        delete it->second;
    }
    mymap.insert(std::make_pair("VAL",new MonDouble(1.5)));

    //delete the value (Part 3)
    it = mymap.find("VAL");
    if(it != mymap.end()){
        delete it->second;
    }
}

using valgrind :

valgrind --leak-check=yes main

I have "Invalid free() / delete / delete[]" errors on deletes of Part 2 and 3.
If I remove Part 2 (the modification), I have no error.

I don't see the error I made. Do you have the same problem ?
In advance, thank you for any answer

[laserlight]

For starters, why not use a std::map<std::string,MonDouble> instead?

As for your problem: you did not erase the element from the map, and then attempted to insert another element with the same key. The insertion should have failed, but as you made no attempt to check for this, you went ahead with the find and delete, resulting in double deletion.

[rilpo]

Thank you laserlight, to be so fast :-)

First this is a simplified example, and actually a really need to store pointers and not objects.

Second, I'm not sure to understand. But are you saying that, by using delete it->second, I only delete the MonDouble instantiation and not the pair<string,MonDouble*> of the map ?

[Lindley]

First this is a simplified example, and actually a really need to store pointers and not objects.

In that case, I recommend you consider using smart pointers.

Second, I'm not sure to understand. But are you saying that, by using delete it->second, I only delete the MonDouble instantiation and not the pair<string,MonDouble*> of the map ?

Correct.

[rilpo]

Ok thank you,
I use "mymap.erase(it)" just after the delete it->second and it works ..
But I'm not sure this is most efficient..

Smart pointers have been abandoned in my project (not my choice :-))
Thank you again laserlight.

[Lindley]

Without using smart pointers it's hard to do better for the erase portion. However, for the modify step you don't need to remove and recreate the mapping because only the value is changing, not the key:

//modify the value (Part 2)
    it = mymap.find("VAL");
    if(it != mymap.end()){
        delete it->second;
        it->second = new MonDouble(1.5);
    }
    else
        mymap.insert(std::make_pair("VAL",new MonDouble(1.5)));

[rilpo]

Yes this solution is better,
Thank you lindley.

[itsmeandnobodyelse]

First this is a simplified example, and actually a really need to store pointers and not objects.

A pointer will have same size as a double with newer compilers. Why should there be a need to store a pointer rather than an object (a copy)?

If you want to store the same variable into multiple containers and therefore have to use pointers, in my opinion you make a design error where I know not one single case where this really must be done that way. When the pointer must be deleted you need to update all the containers where the pointer is stored anyway. So you could do an update in multiple containers as well if storing a copy of the same object into multiple containers (if that really was necessary at all where I have serious doubts about.

On the contrary, storing pointers to containers nearly always give problems like the one you did encounter here and storing them to multiple containers always will lead to highly error-prone software which only can be maintained badly. I can see from your response that you already know that and the sooner you would make a new design the sooner you would solve the problems that arise only because of this.

[Ledidas]

For starters, why not use a std::map<std::string,MonDouble> instead?

As for your problem: you did not erase the element from the map, and then attempted to insert another element with the same key. The insertion should have failed, but as you made no attempt to check for this, you went ahead with the find and delete, resulting in double deletion.

I perceive using a pointer or a reference is just the same but coders of segmented lib code love to read real values rather than handles. What library is that actually ?

[laserlight]

I perceive using a pointer or a reference is just the same

Your perception is not reality. For starters, a reference type cannot be used. With a pointer, there is the question of ownership; with a value, the container owns the object.

coders of segmented lib code love to read real values rather than handles. What library is that actually ?

Huh?

[rilpo]

A pointer will have same size as a double with newer compilers. Why should there be a need to store a pointer rather than an object (a copy)?

Actually, itsmeandnobodyelse, in this software, pointers are here to handle polymorphism.
The container can contains MyDouble, or MyInt.. that inherit MyValue.
When possible we try not to use pointers. To many memory errors can occur with pointers.

Note : I really appreciate these forums; always someone to talk to :-) Thanks to you guys

[D_Drmmr]

Actually, itsmeandnobodyelse, in this software, pointers are here to handle polymorphism.
The container can contains MyDouble, or MyInt.. that inherit MyValue.
When possible we try not to use pointers. To many memory errors can occur with pointers.

If you want to store one of a limited set of built-in types, consider using boost::variant.

[itsmeandnobodyelse]

pointers are here to handle polymorphism. The container can contains MyDouble, or MyInt.. that inherit MyValue.

Yes, it might be senseful to store baseclass pointers and function pointers into containers. But you also could make a Variant type which stores the pointer and handles all creation/allocation/freeing issues and especially handles copy paradigm. That way your map has a normal object to store and you have the polymorphism functionality well encapsulated into a C++ class.

[Pocoya]

Your perception is not reality. For starters, a reference type cannot be used. With a pointer, there is the question of ownership; with a value, the container owns the object.

Hello, how do you define "starters" ? I'm embarassed, I'm a starter of C++, and also learned use of references and pointers in basics C++, could you give an example of the difference you stated ?

[Lindley]

References cannot be used in containers because they are not assignable. Once a reference is pointing to something, it cannot be redirected to point at something else.