C++ Memory Leaks with my std::map of &Objects and std::vector of &Objects

Question

Basically I have this

std::map<std::string, Location&> exits = std::map<std::string, Location&>();

As a private member in a class. And I am unsure about how I would delete it to free the memory when the object of the class gets deleted

I also have a lot of vectors like this

std::vector<Item> Ritems;

Which I am also unsure how to delete, the vector gets Objects& added to it

Deleaker gives me around 1000 of the following:

xmemory0, line 89 (c:\program files (x86)\microsoft visual studio 14.0\vc\include\xmemory0)

Location object

class Object;
class Location
{
    public:
        Location();
        Location(std::string RoomName, std::string RoomDesc);
        ~Location();
        Location(const Location& e);
        void AddExit(std::string Direction, Location &Room);
        void AddItem(Item &Items);
        void AddObject(Object &Objects);
        void RemoveObject(std::string ObjName);
        void AddNPC(NPC &NPCs);
        void PickUpItem(Character &CurChar, std::string ItemName);
        void DisplayAll();
        void DisplayExits();
        void DisplayItems();
        void DisplayObjects();
        void DisplayNPCs();
        std::string GetName();
        std::string GetDesc();
        Location GoCommand(std::string Direction);
        void TalkCommand(std::string Communication, Character &MainCharacter);
        Location operator=(const Location &other);
        Object CheckObject(std::string Command, std::string ObjName);
    private:
        std::string Name;
        std::string Description;

        std::map<std::string, Location&> exits = std::map<std::string, Location&>();

        std::vector<Item> Ritems;
        std::vector<Object> Robjects;
        std::vector<NPC> RNPC;
};

#include <iostream>
#include "Locations.h"  
#include <regex>
#include "Object.h"

Location::Location()
{
    Name = "";
    Description = "";
}
Location::Location(std::string RoomName, std::string RoomDesc)
{
    Name = RoomName;
    Description = RoomDesc;
}
Location::~Location()
{

}
Location::Location(const Location& e)
{
    Name = e.Name;
    Description = e.Description;
    exits = e.exits;
    Ritems = e.Ritems;
    Robjects = e.Robjects;
    RNPC = e.RNPC;
}
void Location::AddExit(std::string Direction, Location &Room)
{
    exits.insert(std::pair<std::string, Location*>(Direction, &Room));
}
void Location::AddItem(Item &Items)
{
    Ritems.push_back(Items);
}
void Location::AddObject(Object &Objects)
{
    Robjects.push_back(Objects);
}
void Location::RemoveObject(std::string ObjName)
{
    Object Temp;
    std::transform(ObjName.begin(), ObjName.end(), ObjName.begin(), ::tolower);
    for (int i = 0; i < Robjects.size(); i++)
    {
        std::string TempS = Robjects[i].GetName();
        std::transform(TempS.begin(), TempS.end(), TempS.begin(), ::tolower);
        if (TempS == ObjName)
            Robjects.erase(Robjects.begin() + i);
    }
}
void Location::AddNPC(NPC &NPCs)
{
    RNPC.push_back(NPCs);
}
void Location::PickUpItem(Character &CurChar, std::string ItemName)
{
    std::transform(ItemName.begin(), ItemName.end(), ItemName.begin(), ::tolower);

    for (int i = 0; i < Ritems.size(); i++)
    {
        std::string Temp = Ritems[i].GetName();
        std::transform(Temp.begin(), Temp.end(), Temp.begin(), ::tolower);
        if (Temp == ItemName)
        {
            CurChar.AddItem(Ritems[i]);
            Ritems.erase(Ritems.begin() + i);
        }
    }
}
Object Location::CheckObject(std::string Command, std::string ObjName)
{
    Object Temp;
    std::transform(Command.begin(), Command.end(), Command.begin(), ::tolower);
    std::transform(ObjName.begin(), ObjName.end(), ObjName.begin(), ::tolower);
    for (int i = 0; i < Robjects.size(); i++)
    {
        std::string TempS = Robjects[i].GetName();
        std::transform(TempS.begin(), TempS.end(), TempS.begin(), ::tolower);
        if (TempS == ObjName)
            return Robjects[i];
    }
    return Temp;
}
void Location::DisplayAll()
{
    WriteLine(7, '-');
    DisplayElement(7, Description);
    DisplayExits();
    DisplayItems();
    DisplayObjects();
    DisplayNPCs();
    WriteLine(7, '-');
}
void Location::DisplayExits()
{
    DisplayElement(7, "|- You can travel; ");

    for (std::map<std::string, Location*>::iterator ii = exits.begin(); ii != exits.end(); ++ii)
    {
        SetColour(7);
        std::cout << "\t";
        SetColour(112);
        std::cout << "[" << (*ii).first << "]";
        SetColour(8);
        std::cout << " to " << (*ii).second->GetName() << std::endl;
    }
}
void Location::DisplayItems()
{
    int Count = 0;
    if (Ritems.size() != 0)
    {
        DisplayElement(7, "Items in room: ");
        for (int i = 0; i < Ritems.size(); i++)
        {
            DisplayElementWC(Count, 5, 13, Ritems[i].GetName());
            DisplayElementWC(Count, 6, 14, Ritems[i].GetDesc());
            DisplayElementWC(Count, 6, 14, Ritems[i].GetItemValue());
            Count++;
        }
    }
}
void Location::DisplayObjects()
{
    int Count = 0;
    if (Robjects.size() != 0)
    {
        DisplayElement(7, "Objects in room: ");
        for (int i = 0; i < Robjects.size(); i++)
        {
            DisplayElementWC(Count, 5, 13, Robjects[i].GetName());
            DisplayElementWC(Count, 6, 14, Robjects[i].GetDesc());
        }
    }
}
void Location::DisplayNPCs()
{
    int Count = 0;
    if (RNPC.size() != 0)
    {
        DisplayElement(7, "NPCs in room: ");
        for (int i = 0; i < RNPC.size(); i++)
        {
            DisplayElementWC(Count, 5, 13, RNPC[i].GetName());
            DisplayElementWC(Count, 6, 14, RNPC[i].GetDesc());
        }
    }
}
std::string Location::GetName()
{
    return Name;
}
std::string Location::GetDesc()
{
    return Description;
}

Location Location::GoCommand(std::string Direction)
{
    Location ReturnLoc = *this;
    std::string Test;
    std::transform(Direction.begin(), Direction.end(), Direction.begin(), ::tolower);
    for (std::map<std::string, Location*>::iterator ii = exits.begin(); ii != exits.end(); ++ii)
    {
        Test = (*ii).first;
        std::transform(Test.begin(), Test.end(), Test.begin(), ::tolower);
        if (Test == Direction)
            ReturnLoc = *(*ii).second;
    }
    return ReturnLoc;
}
void Location::TalkCommand(std::string Communication, Character &MainCharacter)
{
    std::string Test;
    std::transform(Communication.begin(), Communication.end(), Communication.begin(), ::tolower);
    for (int i = 0; i < RNPC.size(); i++)
    {
        Test = RNPC[i].GetName();
        std::transform(Test.begin(), Test.end(), Test.begin(), ::tolower);
        if (Test == Communication)
        {
            RNPC[i].StartConvo(MainCharacter);
        }
    }
}
Location Location::operator=(const Location &other)
{
    Name = other.Name;
    Description = other.Description;
    exits = other.exits;
    Ritems = other.Ritems;
    Robjects = other.Robjects;
    RNPC = other.RNPC;
    return *this;
}

Okay I hope this is a MCVE aha

#include <iostream>
#include <map>
#include <regex>
#include <string>
#include <windows.h>
#include <cctype>
//Custom Classes

class Location;
class UpdateLocation
{
public:
    UpdateLocation();
    ~UpdateLocation();
    void AddLocation(Location &Room);
    void UpdateNow(Location &Room);
    Location GetLocal(Location &Room);

private:
    std::map<std::string, Location*> Locations = std::map<std::string, Location*>();
};

class Location
{
public:
    Location();
    Location(std::string RoomName, std::string RoomDesc);
    ~Location();
    Location(const Location& e);
    void AddExit(std::string Direction, Location &Room);
    void DisplayExits();
    std::string GetName();
    std::string GetDesc();
    Location operator=(const Location &other);
private:
    std::string Name;
    std::string Description;

    std::map<std::string, Location*> exits = std::map<std::string, Location*>();
};

UpdateLocation::UpdateLocation()
{

}
UpdateLocation::~UpdateLocation()
{

}
void UpdateLocation::AddLocation(Location &Room)
{
    Locations.insert(std::pair<std::string, Location*>(Room.GetName(), &Room));
}
void UpdateLocation::UpdateNow(Location &Room)
{
    for (std::map<std::string, Location*>::iterator ii = Locations.begin(); ii != Locations.end(); ++ii)
    {
        if ((*ii).first == Room.GetName())
        {
            *(*ii).second = Room;
        }
    }
}
Location UpdateLocation::GetLocal(Location &Room)
{
    for (std::map<std::string, Location*>::iterator ii = Locations.begin(); ii != Locations.end(); ++ii)
    {
        if ((*ii).first == Room.GetName())
        {
            return *(*ii).second;
        }
    }
}

Location::Location()
{
    Name = "";
    Description = "";
}
Location::Location(std::string RoomName, std::string RoomDesc)
{
    Name = RoomName;
    Description = RoomDesc;
}
Location::~Location()
{

}
Location::Location(const Location& e)
{
    Name = e.Name;
    Description = e.Description;
    exits = e.exits;

}
void Location::AddExit(std::string Direction, Location &Room)
{
    exits.insert(std::pair<std::string, Location*>(Direction, &Room));
}
void Location::DisplayExits()
{
    std::cout << "|- You can travel; " << std::endl;

    for (std::map<std::string, Location*>::iterator ii = exits.begin(); ii != exits.end(); ++ii)
    {
        std::cout << "\t";
        std::cout << "[" << (*ii).first << "]";
        std::cout << " to " << (*ii).second->GetName() << std::endl;
    }
}
std::string Location::GetName()
{
    return Name;
}
std::string Location::GetDesc()
{
    return Description;
}
Location Location::operator=(const Location &other)
{
    Name = other.Name;
    Description = other.Description;
    exits = other.exits;
    return *this;
}

void main()
{
    //Create GameWorld
    UpdateLocation UpdateIt;
    Location HallWay("Hallway", "Long corridor with a wide array of footboats");

    getchar();
    getchar();
}

I don't understand where all the 1000 leaks could be coming from then? — Lazurus, Jan 24 '16 at 23:15
Are you allocating any memory in `Item`? Perhaps you did not properly handle the rule of 3. — drescherjm, Jan 24 '16 at 23:16
You only `delete` things that you create with `new`. You do not delete containers unless you created them with `new`, and you do not delete elements of container unless you created them with `new`. That said, your design suggests some confusion about object ownership, and should be changed so that ownership is clear and explicit. Also, your MCVE doesn't show any leaks because it doesn't _do_ anything. You never add anything to the containers. You forgot the part of the MCVE that actually demonstrates the problem. — Jonathan Wakely, Jan 25 '16 at 14:23
Also, what is `Location UpdateLocation::GetLocal(Location &Room)` supposed to return if the room is not found? And if it returns by value, presumably object identity doesn't matter, so why are you storing references or pointers in your map anyway? Just use `map` and stop causing problems for yourself. — Jonathan Wakely, Jan 25 '16 at 14:37

score 3 · Answer 1 · edited May 23 '17 at 12:30

3

First, you probably want to read this (for c++03 and earlier):

Why Can't I store references in an STL map in C++?

For c++11 and later it's actually possible to have references as values in std::maps using std::map::emplace(), but it's inconvenient and I cannot see it being as useful as raw pointers, which also should be replaced with std::unique_ptrs if the container object owns the objects placed in it.

You probably want

std::map<std::string, Location *> exits;

as your private member. There's no need to delete your map nor your vector. When the destructor of your class is called, the destructor of the respective object is called. They basically self-destruct. You explained that the exits object doesn't own the Location objects, so exits shouldn't have anything to do with deallocating memory allocated for them.

edited May 23 '17 at 12:30

Community

1
1

answered Jan 24 '16 at 23:21

apriori

1,260
11
29

This didnt work, as when i moved Location to another the data wasnt there, i have added the location definitions to my original post – Lazurus Jan 24 '16 at 23:27
Check your implementations of `Location::Location(const Location &)` and `Location Location::operator=(const Location &)` then. Or try removing them and see what the default ones do... – apriori Jan 24 '16 at 23:44
What is the purpose of the `exits` member variable? – apriori Jan 24 '16 at 23:49
Well the locations are linked by a direction which you 'move' down to get to the other locations. The exits member variable stores the location of the other location. – Lazurus Jan 24 '16 at 23:50
I think I understand. Each `Location` keeps the `Location`s adjacent to it. To get this working, and if you can be sure that the values in `exits` aren't used after single `Location` objects are deleted, I'd suggest using a raw pointer: `std::map`. You might want to use an `enum` in conjunction with an `std::array` if you run into speed issues with the `std::map` access, but without actual measurements, that would be a premature optimization. The raw pointer could also be replaced later by a `std::shared_ptr` to make it more robust. – apriori Jan 24 '16 at 23:56
Okay this change has been made now, but there is still 1082 leaks as before – Lazurus Jan 25 '16 at 00:01
Erm Im unsure what parts I would provide? Sorry New to stackoverflow – Lazurus Jan 25 '16 at 00:13
See the link I provided. You need to provide a minimal piece of code that is compilable and exhibits the problem. Otherwise it's guesswork to find the cause - even an innocent looking `void Location::DisplayObjects()` could call `asprintf()` without `free()`ing the allocated memory. – apriori Jan 25 '16 at 00:17
I dont think i can minimilise it very easily, i can add the information from the Location.cpp if that would help? – Lazurus Jan 25 '16 at 00:25
You can try to provide as much code as possible, but you'll definitely get much more people to help you when you provide a MCVE. It's pretty customary around here. – apriori Jan 25 '16 at 00:32
Im trying to put one together atm, i will add the Location.cpp information to my original post – Lazurus Jan 25 '16 at 00:35
okay I have added a MCVE hopefully aha its in the original post – Lazurus Jan 25 '16 at 00:48
The MCVE does not leak memory, according to valgrind. – apriori Jan 25 '16 at 01:00
For my the MCVE says it has 10 leaks well according to Deleaker – Lazurus Jan 25 '16 at 01:02
10 leaks with only calling one empty and one almost empty constructor? GCC does not even compile the line `std::map Locations = std::map();`. Have you tried changing it to `std::map Locations{}`? Same goes for `std::map exits = std::map()`. – apriori Jan 25 '16 at 01:13
okay just tried this and still the same, one of the leaks is the getchar() – Lazurus Jan 25 '16 at 01:20
With `getchar()` leaking, I suspect that the problems are not in your code. I don't have any experience with Deleaker though. I would just ignore what Deleaker says, keep on working on the code and see if a pattern emerges. You could use a debugger to step through your program and watch memory usage and call `exit()` after sensible points and watch how the reported number of leaks changes. – apriori Jan 25 '16 at 01:24
Yeah it claims that the system is leaking at '_Ptr = ::operator new(_User_size);' in xmemory() at nearly all the other leaks, and when all the other stuff in the main project is there then there are around 1000 instead of 9 – Lazurus Jan 25 '16 at 01:26
I'd probably try a different leak detector first. A call trace of the allocation would be beneficial too. – apriori Jan 25 '16 at 01:40

C++ Memory Leaks with my std::map of &Objects and std::vector of &Objects

1 Answers1