How to empty std::set from objects' pointers?

Question

I am having an issue emptying my set, so I have 3 classes like so:
class A, and 2 inherited classes B and C. In the code I store elements in my set from the 3 types, the set is:

set<A*> objects;

so whenever I create a B element I do that:

A* b = new B(); // calling B C'tor 

// and so on with A and C elements I do the exact same.`

So here comes the problem, whenever I want to erase an element, or even end the program (which calls the destructor), I don't know what I should type in the destructor, I have it like that:

set<A*>::iterator first = objects.begin();
set<A*>::iterator last = objects.end();
while (first != last) {
    set<A*>::iterator to_delete = first;
    objects.erase(to_delete);
    delete *to_delete;    
    ++first;
}

I have also tried putting the delete *to_delete; above objects.erase , also tried putting it alone ,and tried putting the erase alone without delete, but the thing is I have used new so I should use delete somewhere. all things aren't working, the program just crashes with that, I tried keeping the D'tor empty, the program works, but I have a lot of memory leaks, I have checked that.

Please help me, I stuck with this thing. Thank you very much <3

The file: Everything works perfectly except for the Destructor, and the removeRoom function (basically where is a delete.. also operator<< isn't working properly but I believe it's because of that thing, again I have virtual ~EscapeRoomWrapper();

#include <string>
#include <iostream>
#include <set>
#include "Company.h"
#include "ScaryRoom.h"
#include "KidsRoom.h"
#include "Exceptions.h"

using std::set;
using std::endl;

using namespace mtm;
using namespace escaperoom;

Company::Company(string name, string phoneNumber) : name(name), phoneNumber(phoneNumber) {

}

Company::~Company() {
    while(!rooms.empty()) {
        set<EscapeRoomWrapper*>::iterator iter = rooms.begin();
        rooms.erase(iter);
        delete *iter;
    }
//  set<EscapeRoomWrapper*>::iterator first = rooms.begin();
//  set<EscapeRoomWrapper*>::iterator last = rooms.end();
//  while (first != last) {
//      set<EscapeRoomWrapper*>::iterator to_delete = first;
//      rooms.erase(to_delete);
//      delete *to_delete;
//
//      ++first;
//      last = rooms.end();
//  }
//  while (rooms.begin() != rooms.end()) {
//      set<EscapeRoomWrapper*>::iterator to_delete = rooms.begin();
//      rooms.erase(to_delete);
//      delete *to_delete;
//  }
}

Company::Company(const Company& company) : name(company.name), phoneNumber(company.phoneNumber), rooms(company.rooms) {

}

Company& Company::operator=(const Company& company) {
    if (this == &company) {
        return *this;
    }
    name = company.name;
    phoneNumber = company.phoneNumber;
    rooms.clear();
    rooms = company.rooms;
    return *this;
}

void Company::createRoom(char* name, const int& escapeTime, const int& level, const int& maxParticipants) {
    try {
        EscapeRoomWrapper* room = new EscapeRoomWrapper(name, escapeTime, level, maxParticipants);
        rooms.insert(room);
    } catch (EscapeRoomMemoryProblemException& e) {
        throw CompanyMemoryProblemException();
    }
}

void Company::createScaryRoom(char* name, const int& escapeTime, const int& level,
                                 const int& maxParticipants, const int& ageLimit, const int& numOfScaryEnigmas) {
    try {
        EscapeRoomWrapper* room = new ScaryRoom(name, escapeTime, level, maxParticipants, ageLimit, numOfScaryEnigmas);
        rooms.insert(room);
    } catch (EscapeRoomMemoryProblemException& e) {
        throw CompanyMemoryProblemException();
    }
}

void Company::createKidsRoom(char* name, const int& escapeTime, const int& level,
        const int& maxParticipants, const int& ageLimit) {
    try {
        EscapeRoomWrapper* room = new KidsRoom(name, escapeTime, level, maxParticipants, ageLimit);
        rooms.insert(room);
    } catch (EscapeRoomMemoryProblemException& e) {
        throw CompanyMemoryProblemException();
    }
}

set<EscapeRoomWrapper*> Company::getAllRooms() const {
    return rooms;
}

void Company::removeRoom(const EscapeRoomWrapper& room) {
    set<EscapeRoomWrapper*>::iterator first = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last = rooms.end();
    while (first != last) {
        if (**first == room) {
            break;
        }
        ++first;
    }
    if (first == last) {
        throw CompanyRoomNotFoundException();
    }
    delete *first;
    rooms.erase(first);
   // delete *first;     // check this
}

void Company::addEnigma(const EscapeRoomWrapper& room, const Enigma& enigma) {
    set<EscapeRoomWrapper*>::iterator first = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last = rooms.end();
    while (first != last) {
        if (**first == room) {
            break;
        }
        ++first;
    }
    if (first == last) {
        throw CompanyRoomNotFoundException();
    }
    (**first).addEnigma(enigma);
}

void Company::removeEnigma(const EscapeRoomWrapper& room, const Enigma& enigma) {
    set<EscapeRoomWrapper*>::iterator first = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last = rooms.end();
    while (first != last) {
        if (**first == room) {
            break;
        }
        ++first;
    }
    if (first == last) {
        throw CompanyRoomNotFoundException();
    }
    try {
        (**first).removeEnigma(enigma);
    } catch (EscapeRoomNoEnigmasException& e) {
        throw CompanyRoomHasNoEnigmasException();
    } catch (EscapeRoomEnigmaNotFoundException& e) {
        throw CompanyRoomEnigmaNotFoundException();
    }
}

void Company::addItem(const EscapeRoomWrapper& room, const Enigma& enigma, const string& element) {
    set<EscapeRoomWrapper*>::iterator first_room = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last_room = rooms.end();
    while (first_room != last_room) {
        if (**first_room == room) {
            break;
        }
        ++first_room;
    }
    if (first_room == last_room) {
        throw CompanyRoomNotFoundException();
    }
    vector<Enigma>::iterator first = (**first_room).getAllEnigmas().begin();
    vector<Enigma>::iterator last = (**first_room).getAllEnigmas().end();
    while (first != last) {
        if (*first == enigma) {
            break;
        }
        ++first;
    }
    if (first == last) {
        throw CompanyRoomEnigmaNotFoundException();
    }
    (*first).addElement(element);
}

void Company::removeItem(const EscapeRoomWrapper& room, const Enigma& enigma, const string& element) {
    set<EscapeRoomWrapper*>::iterator first_room = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last_room = rooms.end();
    while (first_room != last_room) {
        if (**first_room == room) {
            break;
        }
        ++first_room;
    }
    if (first_room == last_room) {
        throw CompanyRoomNotFoundException();
    }
    vector<Enigma>::iterator first = (**first_room).getAllEnigmas().begin();
    vector<Enigma>::iterator last = (**first_room).getAllEnigmas().end();
    while (first != last) {
        if (*first == enigma) {
            break;
        }
        ++first;
    }
    if (first == last) {
        throw CompanyRoomEnigmaNotFoundException();
    }
    try {
        (*first).removeElement(element);
    } catch (EnigmaNoElementsException& e) {
        throw CompanyRoomEnigmaHasNoElementsException();
    } catch (EnigmaElementNotFoundException& e) {
        throw CompanyRoomEnigmaElementNotFoundException();
    }
}

set<EscapeRoomWrapper*> Company::getAllRoomsByType(RoomType type) const {
    set<EscapeRoomWrapper*> filtered_set;
    set<EscapeRoomWrapper*>::iterator first_room = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last_room = rooms.end();
    EscapeRoomWrapper* ptr = NULL;
    while (first_room != last_room) {
        if (type == BASE_ROOM) {
            if (dynamic_cast<ScaryRoom*>(*first_room) == ptr
                    && dynamic_cast<KidsRoom*>(*first_room) == ptr) {
                filtered_set.insert(*first_room);
            }
        }
        if (type == SCARY_ROOM) {
            if (dynamic_cast<ScaryRoom*>(*first_room) != ptr) {
                filtered_set.insert(*first_room);
            }
        }
        if (type == KIDS_ROOM) {
            if (dynamic_cast<KidsRoom*>(*first_room) != ptr) {
                filtered_set.insert(*first_room);
            }
        }
        ++first_room;
    }
    return filtered_set;
}

EscapeRoomWrapper* Company::getRoomByName(const string& name) const {
    set<EscapeRoomWrapper*>::iterator first_room = rooms.begin();
    set<EscapeRoomWrapper*>::iterator last_room = rooms.end();
    while (first_room != last_room) {
        if ((**first_room).getName() == name) {
            break;
        }
        ++first_room;
    }
    if (first_room == last_room) {
        throw CompanyRoomNotFoundException();
    }
    return *first_room;
}

std::ostream& mtm::escaperoom::operator<<(std::ostream& output, const Company& company) {
    output << company.name << " : " << company.phoneNumber << endl;
    set<EscapeRoomWrapper*>::iterator first_room = company.getAllRooms().begin();
    set<EscapeRoomWrapper*>::iterator last_room = company.getAllRooms().end();
    while (first_room != last_room) {
        output << **first_room << endl;
        ++first_room;
    }
    return output;
}

Answer 1

I believe your question is an example of an XY problem: You want to empty your set of pointers, but you only want that because you want to manually destruct that set without leaking memory. And you only need to do that because automatic destruction won't take of that for you.

The real solution, as I see it, is to avoid the manual allocations with new. What would you do instead? There are probably 3 options:

1. unique_ptr's
2. shared_ptr's
3. std::variant's

I'm guessing you don't really need to assign one company to another while keeping both; since your assignment operator and copy constructor semantics are wrong: you're making one company's rooms modifiable through the other company which is supposed to be const. You probably just perform move constructor:

Company::Company(Company&& company);

in which you would 'grab' the old company's room set, leaving it empty (and maybe a move-assignment operator). If that's the case - you only ever have hold one reference to a room, and unique_ptr<EscapeRoomWrapper> will do. You will not have to manually delete anything, and the default destructor for the set will work just fine. In fact, you might just be able to drop the custom move constructor altogether and just use the default.

If you do need multiple companies to refer to the same room, and do need those non-rvalue constructor and assignment operator - use shared_ptr<EscapeRoomWrapper>. Again, you won't need to delete anything, and the rooms will be deleted when the last reference to them is destructed. This will cost you a bit of overhead, but this isn't performance-critical code anyway so it doesn't matter.

Finally, if there is a limited variety of rooms, and they all inherit from the wrapper class - you might consider replacing the virtual inheritance with an std::variant of the different room classes, and not using pointer at all - just having a set of room.

For additional inspiration for my suggestions, consider reading about the rule of zero, e.g. in this blog post.

PS - Are you sure you need the set of rooms to be ordered? If not, use std::unordered_set instead of std::set.

Answer 2

The problem is that objects.end() changes when something is removed from the set and the value stored in last is invalidated.
You can fix your code as follows:

    while (std::begin(objects) != std::end(objects)) {
        set<A*>::iterator to_delete = objects.begin();
        objects.erase(to_delete);
        delete *to_delete;
    }

---

In general you shouldn't use raw pointers at all in the set. Rather use something like

std::set<std::unique_ptr<A>> objects;

in your program. So you don't need to care for the correct deallocation of the objects at all.

Answer 3

The key problem with your approach is the fact that your modifying your container while iterating over it. I'd suggest to refactor it to:

while (!objects.empty()) {
     set<A*>::iterator it = objects.begin();
     objects.erase(it);
     delete *it;
}

Alternatively you can do something like this with C++11 and lamdas:

std::for_each(objects.begin(), objects.end(), [](A* obj){ delete obj; });
objects.clear();

---

Just tested on simplified version based on your description, following snippet works for me pretty well:

#include <iostream>
#include <set>

using namespace std;


class A {
};

class B : public A {
};

int main(int argc, const char *argv[])
{
    set<A*> objects;

    for (int i = 0; i < 10; i++) {
        objects.insert(new B());
    }

    for(set<A*>::iterator it = objects.begin(); it != objects.end(); ++it) {
        delete *it;
    }
    objects.clear();
    cout << "Hello World" << endl;
    return 0;
}

I would suspect we missing some details here.

---

UPDATE

Ok, while it's hard to see the whole picture of what are you trying to do here as most of the details are still missing, I spotted one potential problem with copy constructor. In your updated code you are doing shallow copy of the Company object, but I think that you meant to do is:

Company& Company::operator=(const Company& company) {
    if (this == &company) {
        return *this;
    }
    name = company.name;
    phoneNumber = company.phoneNumber;
    // Also clear might be not enough since you also need to release heap memory

    //rooms.clear();

    while (!rooms.empty()) {
       set<A*>::iterator it = rooms.begin();
       rooms.erase(it);
       delete *it;
    }

    // Deep copy of set of rooms in company object
    for (set<Room*>::iterator it = company.rooms.begin(); it != company.rooms.end(); ++i ) {
       rooms.insert(new Room(*it));
    }
    return *this;
}

Answer 4

If you just want to delete everything, then there's a much simpler solution. Delete everything first, then call clear(). You can turn the deletion part into a one-liner using std::for_each. Here is an example:

#include <algorithm>
// ...

std::for_each(begin(objects), end(objects), [](auto ptr) { delete ptr; });
objects.clear();

Note that if objects goes out of scope immediately afterwards, then clear() isn't even necessary.

If you want to remove a single element, then delete the pointer first and then erase it:

delete *iter; // decltype(iter) = std::set<A*>::iterator
objects.erase(iter);

Also consider using std::unique_ptr instead of raw pointers.