1

I have a method that returns a new map pointer which is later consumed. Where the map is consumed I have the following code:

void Updater::set_data(map<string, string> * data)
{
    if (this->data != NULL)
    {
        delete this->data;
    }
    this->data = data;
}

This consumer is running as long as the application is running.

However, my application is leaking memory and Valgrind is reporting that it's leaking (600 (48 direct, 552 indirect) bytes in 1 blocks are definately lost in loss record ...) at the line that constructs the map:

map<string, string> * values = new map<string, string>();

Am I doing something wrong to get rid of the map (and free the memory)? I don't have much C++ experience, so I'm guessing this is some beginners mistake, but I've spend the larger amount of a day trying to figure what it could be.

EDIT I have only 2 updaters running (or 1 if I chose so, but always predefined fixed amount) and the map always contains similar data, quite often even exactly the same data as before.

mycroes
  • 645
  • 8
  • 20

3 Answers3

5

You have a few solutions here (if indeed, you are not deleting the last map):

  • add a delete data; in the destructor of Updater.

  • replace the "by pointer" implementation with a "by value" implementation (much better).

Declaration:

class Updater {
    std::map<std::string,std::string> data; // not a pointer
    //...
};

Implementation:

void Updater::set_data(map<string, string> data) // <-- by value, not by pointer
{
    this->data = std::move(data);
}

Client code:

Updater u; // updater instance

map<string, string> values;
// fill values here
u.set_data(std::move(values));
utnapistim
  • 26,809
  • 3
  • 46
  • 82
4

From what you've told us, you delete the intermediate maps as your program runs, but you never delete the last one you create before your program shuts down. That's probably what you're missing. All this would go away if you used smart pointers.

John Zwinck
  • 239,568
  • 38
  • 324
  • 436
2

Step #1 - Make sure that you set data = NULL in every constructor, for example:

Updater::Updater()
{
    data = NULL;
}

Step #2 - Add a destructor:

Updater::~Updater()
{
    if (data != NULL)
    {
        delete data;
    }
}

BTW, as suggested in some of the comments above, you might be better off with a statically-allocated instance of map<string,string> rather than a pointer to a dynamically-allocated instance of this type...

barak manos
  • 29,648
  • 10
  • 62
  • 114