Dynamically allocating an array of objects

Question

I have a class that contains a dynamically allocated array, say

class A
{
    int* myArray;
    A()
    {
        myArray = 0;
    }
    A(int size)
    {
        myArray = new int[size];
    }
    ~A()
    {
        // Note that as per MikeB's helpful style critique, no need to check against 0.
        delete [] myArray;
    }
}

But now I want to create a dynamically allocated array of these classes. Here's my current code:

A* arrayOfAs = new A[5];
for (int i = 0; i < 5; ++i)
{
    arrayOfAs[i] = A(3);
}

But this blows up terribly. Because the new A object created (with the A(3) call) gets destructed when the for loop iteration finishes, and this means that the internal myArray of that A instance gets delete []-ed.

So I think my syntax must be terribly wrong? I guess there are a few fixes that seem like overkill, which I'm hoping to avoid:

• Creating a copy constructor for A.
• Using vector<int> and vector<A> so I don't have to worry about all this.
• Instead of having arrayOfAs be an array of A objects, have it be an array of A* pointers.

I would think this is just some beginners thing where there's a syntax that actually works when attempting to dynamically allocate an array of things that have internal dynamic allocation.

(Also, style critiques appreciated, since it's been a while since I did C++.)

Update for future viewers: All of the answers below are really helpful. Martin's is accepted because of the example code and the useful "rule of 4," but I really suggest reading them all. Some are good, succinct statements of what's wrong, and some point out correctly how and why vectors are a good way to go.

Solution 1

For building containers you obviously want to use one of the standard containers (such as a std::vector). But this is a perfect example of the things you need to consider when your object contains RAW pointers.

If your object has a RAW pointer then you need to remember the rule of 3 (now the rule of 5 in C++11).

• Constructor
• Destructor
• Copy Constructor
• Assignment Operator
• Move Constructor (C++11)
• Move Assignment (C++11)

This is because if not defined the compiler will generate its own version of these methods (see below). The compiler generated versions are not always useful when dealing with RAW pointers.

The copy constructor is the hard one to get correct (it's non trivial if you want to provide the strong exception guarantee). The Assignment operator can be defined in terms of the Copy Constructor as you can use the copy and swap idiom internally.

See below for full details on the absolute minimum for a class containing a pointer to an array of integers.

Knowing that it is non trivial to get it correct you should consider using std::vector rather than a pointer to an array of integers. The vector is easy to use (and expand) and covers all the problems associated with exceptions. Compare the following class with the definition of A below.

class A
{ 
    std::vector<int>   mArray;
    public:
        A(){}
        A(size_t s) :mArray(s)  {}
};

Looking at your problem:

A* arrayOfAs = new A[5];
for (int i = 0; i < 5; ++i)
{
    // As you surmised the problem is on this line.
    arrayOfAs[i] = A(3);

    // What is happening:
    // 1) A(3) Build your A object (fine)
    // 2) A::operator=(A const&) is called to assign the value
    //    onto the result of the array access. Because you did
    //    not define this operator the compiler generated one is
    //    used.
}

The compiler generated assignment operator is fine for nearly all situations, but when RAW pointers are in play you need to pay attention. In your case it is causing a problem because of the shallow copy problem. You have ended up with two objects that contain pointers to the same piece of memory. When the A(3) goes out of scope at the end of the loop it calls delete [] on its pointer. Thus the other object (in the array) now contains a pointer to memory that has been returned to the system.

The compiler generated copy constructor; copies each member variable by using that members copy constructor. For pointers this just means the pointer value is copied from the source object to the destination object (hence shallow copy).

The compiler generated assignment operator; copies each member variable by using that members assignment operator. For pointers this just means the pointer value is copied from the source object to the destination object (hence shallow copy).

So the minimum for a class that contains a pointer:

class A
{
    size_t     mSize;
    int*       mArray;
    public:
         // Simple constructor/destructor are obvious.
         A(size_t s = 0) {mSize=s;mArray = new int[mSize];}
        ~A()             {delete [] mArray;}

         // Copy constructor needs more work
         A(A const& copy)
         {
             mSize  = copy.mSize;
             mArray = new int[copy.mSize];

             // Don't need to worry about copying integers.
             // But if the object has a copy constructor then
             // it would also need to worry about throws from the copy constructor.
             std::copy(&copy.mArray[0],&copy.mArray[c.mSize],mArray);

         }

         // Define assignment operator in terms of the copy constructor
         // Modified: There is a slight twist to the copy swap idiom, that you can
         //           Remove the manual copy made by passing the rhs by value thus
         //           providing an implicit copy generated by the compiler.
         A& operator=(A rhs) // Pass by value (thus generating a copy)
         {
             rhs.swap(*this); // Now swap data with the copy.
                              // The rhs parameter will delete the array when it
                              // goes out of scope at the end of the function
             return *this;
         }
         void swap(A& s) noexcept
         {
             using std::swap;
             swap(this.mArray,s.mArray);
             swap(this.mSize ,s.mSize);
         }

         // C++11
         A(A&& src) noexcept
             : mSize(0)
             , mArray(NULL)
         {
             src.swap(*this);
         }
         A& operator=(A&& src) noexcept
         {
             src.swap(*this);     // You are moving the state of the src object
                                  // into this one. The state of the src object
                                  // after the move must be valid but indeterminate.
                                  //
                                  // The easiest way to do this is to swap the states
                                  // of the two objects.
                                  //
                                  // Note: Doing any operation on src after a move 
                                  // is risky (apart from destroy) until you put it 
                                  // into a specific state. Your object should have
                                  // appropriate methods for this.
                                  // 
                                  // Example: Assignment (operator = should work).
                                  //          std::vector() has clear() which sets
                                  //          a specific state without needing to
                                  //          know the current state.
             return *this;
         }   
 }

Solution 2

I'd recommend using std::vector: something like

typedef std::vector<int> A;
typedef std::vector<A> AS;

There's nothing wrong with the slight overkill of STL, and you'll be able to spend more time implementing the specific features of your app instead of reinventing the bicycle.

Solution 3

The constructor of your A object allocates another object dynamically and stores a pointer to that dynamically allocated object in a raw pointer.

For that scenario, you must define your own copy constructor , assignment operator and destructor. The compiler generated ones will not work correctly. (This is a corollary to the "Law of the Big Three": A class with any of destructor, assignment operator, copy constructor generally needs all 3).

You have defined your own destructor (and you mentioned creating a copy constructor), but you need to define both of the other 2 of the big three.

An alternative is to store the pointer to your dynamically allocated int[] in some other object that will take care of these things for you. Something like a vector<int> (as you mentioned) or a boost::shared_array<>.

To boil this down - to take advantage of RAII to the full extent, you should avoid dealing with raw pointers to the extent possible.

And since you asked for other style critiques, a minor one is that when you are deleting raw pointers you do not need to check for 0 before calling delete - delete handles that case by doing nothing so you don't have to clutter you code with the checks.

Solution 4

You need an assignment operator so that:

arrayOfAs[i] = A(3);

works as it should.

Solution 5

Why not have a setSize method.

A* arrayOfAs = new A[5];
for (int i = 0; i < 5; ++i)
{
    arrayOfAs[i].SetSize(3);
}

I like the "copy" but in this case the default constructor isn't really doing anything. The SetSize could copy the data out of the original m_array (if it exists).. You'd have to store the size of the array within the class to do that.
OR
The SetSize could delete the original m_array.

void SetSize(unsigned int p_newSize)
{
    //I don't care if it's null because delete is smart enough to deal with that.
    delete myArray;
    myArray = new int[p_newSize];
    ASSERT(myArray);
}

View more solutions

Comments

[Domenic]

So many really good answers; I really want to accept most of them, yours included, as the "best." Thank you very much. And also for the style critique.

[Martin York]

Its the rule of 4. It needs a normal constructor as well. If you do not initialize the pointers then they have random values.

[Martin York]

Actually this uses the assignment operator not the copy constructor. The Left hand side has already been fully constructed.

[Michael Burr]

@Martin - you're right. I've always heard it as the "rule of 3" as the constructor is taken as a 'given'. But I think explicitly including it in the rule is a better way to do it.

[Martin York]

Unfortunately not. Because both the original A(3) and the arrayofAs[i] contain the member myArray that point to the same area on the heap. The first one to go out of scope will delete the object. The second to go out of scope will also delete it, this causes the problem.

[shoosh]

Do you have some like to articles about the exceptions problem you refer to?

[josesuero]

Why do you capitalize "raw"? Surely, it's not an abbreviation for anything, but simply means "raw" as in unmodified, plain, not a smart-pointer or another kind of wrapper.

[joshperry]

@jalf They're called "scare quotes" :)

[Daniele]

why move assignment operator does not return anything?

[Martin York]

@Daniele: Because that's a bug. Fixing.