C++ struct with dynamically allocated char arrays

Question

I'm trying to store structs in a vector. Struct needs to dynamically allocate memory for char* of a given size. But as soon as I add the struct to a vector, its destructor gets called, as if I lost the pointer to it.

I've made this little demo for the sake of example.

#include "stdafx.h"
#include <iostream>
#include <vector>

struct Classroom
{
    char* chairs;

    Classroom() {} // default constructor

    Classroom(size_t size)
    {
        std::cout << "Creating " << size << " chairs in a classroom" << std::endl;
        chairs = new char[size];
    }

    ~Classroom()
    {
        std::cout << "Destroyng chairs in a classroom" << std::endl;
        delete[] chairs;
    }
};

std::vector<Classroom> m_classrooms;

int main()
{

    m_classrooms.push_back(Classroom(29));
    //m_classrooms.push_back(Classroom(30));
    //m_classrooms.push_back(Classroom(30));

    system("Pause");

    return 0;
}

The output is

Creating 29 chairs in a classroom
Destroyng chairs in a classroom
Press any key to continue . . .
Destroyng chairs in a classroom

Yes, seems like the destructor gets called twice! Once upon adding to a vector, and second time upon the program finishing its execution.

The exact same thing happens when I try to use a class instead of a struct.

Can someone explain why this happens and what are the possible ways to accomplish my task correctly?

Answer 1

@LPVOID

Using emplace_back(..) to create the object in place can help you avoid the double free or corruption error you are facing here.

m_classrooms.emplace_back(29)

However, it is a better practice to always follow the rule of 3/5/0 to not end up with a dangling pointer.

Answer 2

The Classroom class cannot be used in a std::vector<Classroom> safely because it has incorrect copy semantics. A std::vector will make copies of your object, and if the copy semantics have bugs, then you will see all of those bugs manifest themselves when you start using the class in containers such as vector.

For your class to have correct copy semantics, it needs to be able to construct, assign, and destruct copies of itself without error (those errors being things like memory leaks, double deletion calls on the same pointer, etc.)

The other thing missing from your code is that the size argument needs to be known within the class. Right now, all you've posted is an allocation of memory, but there is nothing that saves the size. Without knowing how many characters were allocated, proper implementation of the user-defined copy constructor and assignment operator won't be possible, unless that char * is a null-terminated string.

---

Having said that, there a multiple ways to fix your class. The easiest way is to simply use types that have correct copy semantics built into them, instead of handling raw dynamically memory yourself. Those classes would include std::vector<char> and std::string. Not only do they clean up themselves, these classes know their own size without having to carry a size member variable.

struct Classroom
{
    std::vector<char> chairs;

    Classroom() {} // default constructor
    Classroom(size_t size) : chairs(size)
    {
        std::cout << "Creating " << size << " chairs in a classroom" << std::endl;
    }
};

The above class will work without any further adjustments to it, since std::vector<char> has correct copy semantics already. Note that there is no longer a need for the destructor, since std::vector knows how to destroy itself.

---

If for some reason you had to use raw dynamically allocated memory, then your class has to implement a user-defined copy constructor, assignment operation, and destructor.

#include <algorithm>

struct Classroom
{
    size_t m_size;
    char* chairs;

    // Note we initialize all the members here.  This was a bug in your original code
    Classroom() : m_size(0), chairs(nullptr) 
    {} 

    Classroom(size_t size) : m_size(size), chairs(new char[size])
    {}

    Classroom(const Classroom& cRoom) : m_size(cRoom.m_size),
                                        chairs(new char[cRoom.m_size]) 

    {
       std::copy(cRoom.chairs, cRoom.chairs + cRoom.m_size, chairs);
    }

    Classroom& operator=(const Classroom& cRoom)
    {
       if ( this != &cRoom )
       {
          Classroom temp(cRoom);
          std::swap(temp.m_size, m_size);
          std::swap(temp.chairs, chairs);
       }
       return *this;
   }

   ~Classroom() { delete [] chairs; }
};

Note the usage of the member-initialization list when initializing the members of the class. Also note the usage of the copy / swap idiom when implementing the assignment operator.

---

The other issue that was corrected is that your default constructor was not initializing all of the members. Thus in your original class a simple one line program such as:

int main()
{
   Classroom cr;
}

would have caused issues, since in the destructor, you would have deleted an uninitialized chairs pointer.

After this, a std::vector<Classroom> should now be able to be safely used.