Clear map of dynamic values in C++

Question

I've seen many sites talking about the correct way to implement a d'tor for a class that holds a map. But not for the case where the values of the map themselves are dynamically allocated.

For example, let Manager be a class which hold map<int, User*> where User is some class which I'll allocate dynamically later.

By the rules of the exercise, it should handle a registerUser(string name) function, which creates a new User instance and adds it to the map.

Something like:

User* registerUser(std::string userName) {
    User* pNewUser = new User(userName);

    // Setting some stuff

    auto ret = users.insert(std::pair<int, User*>(pNewUser->id, pNewUser));

    // Finishing and returning a pointer to the new allocated User
}

AND TO THE QUESTION ITSELF:

Should the d'tor do something special beyond users.clear()?

Will the memory be freed successfully or shall I iterate over the elements and delete them?

Thank you in advance :)

the destructor should do "something special" but it needs not call `users.clear()`. Can you use smart pointers? — 463035818_is_not_an_ai, Jul 30 '21 at 18:06
Starting with the simplest principle: If you `new` something, you have to `delete` it later. `registerUser` calls `new`, so you have to figure out who's responsible for calling the `delete`. Right now it looks like that's the `Manager` itself. But it's possible to represent this ownership model without using raw pointers and without calling `new` or `delete` at all, as the previous commenter alluded to. You could have a `map>` which would mean that the Manager is solely responsible for them. It would then allow you to greatly simplify your destructor. — Nathan Pierson, Jul 30 '21 at 18:08
I can't use smart pointers, unfortunately, but it's good to know that I need to use `delete` and my suspicions were correct. Thank you! :) — Dniel BV, Jul 30 '21 at 18:11
ok, no smart pointers, but why dynamically allocate the users manually? Why do you not use a `std::map` ? — 463035818_is_not_an_ai, Jul 30 '21 at 18:12
The most correct code would be not have a pointer at all, and instead have a map of ``. As a general rule, owning pointers are appropriate in C++ when run-time polymorphism is involved, and rarely in any other case. — SergeyA, Jul 30 '21 at 18:14
By the exercise, I need to use a map with a pointer to `User`. — Dniel BV, Jul 30 '21 at 18:16
Also be sure to make a copy constructor that properly copies, too. The default one will copy the map and the pointers will alias the same objects as the source, leaving them invalid if the source object is destroyed (once you fix your dtor to delete them!) — Chris Uzdavinis, Jul 30 '21 at 18:18

463035818_is_not_an_ai · Accepted Answer · 2021-07-30T18:43:07.033

Don't use pointers when you do not have to. The std::map already manages the lifetime of its elements for you:

struct User {
    std::string name;
    int id;
    static int id_counter;
    User(const std::string& name) : name(name),id(id_counter++) {}
};

struct manager {
    std::map<int,User> users;

    User& registerUser(std::string userName) {                    
        User u(userName);
        auto ret = users.emplace(u.id,u);
        return ret.first->second;
    }
};

If you are forced to use a std::map<int,User*> because of weird unrealistic exercise requirements (or because the map is supposed to hold polymorphic objects) and you cannot use smart pointers then you need to delete what you newed. The map only manages its elements, not what they might point to:

struct manager {
    std::map<int,User*> users;

    User& registerUser(std::string userName) {               
        User* u = new User(userName);
        auto ret = users.emplace(u->id,u);
        return *(ret.first->second);
    }

    ~manager() {
        for (const auto& user : users){
            delete user.second;
        }
    }

    // the compiler generated assignment and copy would not do the right thing
    manager(const manager&) = delete;
    manager& operator=(const manager&) = delete;
};

Not sure where you read about holding a map as member and needing to call clear(). That is non-sense. The map has a destructor that is called automatically and the map does already clean up after itself.

Last but not least, you need to read about the rule of 3 (What is The Rule of Three?), because a destructor alone is not sufficient to correctly manage raw pointers as members. As mentioned in a comment, when you copy the manager via the compiler generated copy constructor or assignment, bad things will happen. Note that this isnt the case with the first version above. When possible you should try to follow the rule of 0 (https://en.cppreference.com/w/cpp/language/rule_of_three scroll down).

Baumflaum · Answer 2 · 2021-07-30T18:35:38.137

Should the d'tor do something special beyond users.clear()?

In general that depends on how your code handles ownership of heap allocated objects; any time you call a constructor by new (aka allocate on the heap) you should be aware of wich component in your code takes ownership over the newly created object and in consequence is responsible for deletion of the object.

For this specific toyproblem your Manager class should also handle the deletion of the object by iterating over all elements in your favourite way and calling delete. Be aware some other component could still hold onto one of these User-pointers wich will cause a crash by accessing invalid memory in the best case and running fine until it shipped in the worst case (or starting a nuclear war, since this is in the scope of undefined behaviour). The state of the art solution is using some kind of smart pointer.

Of course as 463035818_is_not_a_number put it so nicely in his answer you don't need to call users.clear(). Since the map will be deleted automagically since it is a statically allocated variable (but not necessarily the contents of the map).

Clear map of dynamic values in C++

2 Answers2