CODEWITHSUNDEEP
c++memorydynamicstl
Dniel BV
Dniel BV

Reputation: 68

Clear map of dynamic values in C++

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 :)

Upvotes: 1

Views: 618

Answers (2)

463035818_is_not_an_ai
463035818_is_not_an_ai

Reputation: 122585

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).

Upvotes: 4

Baumflaum
Baumflaum

Reputation: 1385

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).

Upvotes: 1

Related Questions