C++ copy constructor for class with dynamically allocated array

Question

Just starting on C++ (a few days now), I come with C background.

I have a class that holds, mainly, an a pointer to an int array with the following code:

class Array
{
private:
    int * _arr;
    int _size;
public:
    Array();
    Array(int size);
    Array(const Array& obj);  // copy constructor
    ~Array();
    void readInValues();
    void mult(int num);
    void add(int num);
    void printArray();
};

_arr is a pointer to an int array, when creating a new instance using the copy constructor I would to create a new int array on the heap (I think). In the copy constructor:

Array::Array( const Array & obj )
{
    _arr = new int[_size];

    for(int i=0;i<_size;i++)
        *_arr[i] = *obj._arr[i];
}

The 1st thing I do is allocate memory for the new array (_size is a primitive type so being copied automatically as far as I understood). The next thing I would like to do is copy the array itself using a loop. This parts fails the compilation saying illegal indirection. I am not sure why...

Answer 1

This:

Array::Array( const Array & obj )
{
    _arr = new int[_size];

    for(int i=0;i<_size;i++)
        *_arr[i] = *obj._arr[i];
}

could be this:

Array::Array( const Array & obj )
    : _arr(obj._size ? new int[obj._size] : nullptr)
    , _size(obj._size)
{
    if (_arr)
        std::copy(obj._arr, obj._arr+_size, _arr);
}

Specifically, note the use of the initializer list. Other things worth mentioning:

1. Don't use identifiers with leading underscores. Besides making the code a royal pain to read, you can quickly find yourself using identifiers that are reserved by the implementation. That isn't the case here, but I'm willing to bet that wasn't by intent.

2. You also need an assignment operator to fulfill the Rule of Three. Thankfully, a decent copy constructor makes that trivial.

3. Don't do any of this. This is why, with great wailing and much gnashing of teeth in design decisions, the almighty C++ standards committees of years-gone-by have so-chosen to bless us with std::vector<>. If you're doing this to learn, good. What you'll eventually learn is how rarely you need to once you embrace the standard library and all its guilty pleasures.

If this is intended as a learning exercise, kudos, and blissfully ignore (3) above. For (2), you can use the copy/swap idiom now that you have a reasonable copy-ctor:

Array& Array::operator =(Array obj) // value-param intentional
{
    std::swap(_arr, obj._arr);
    std::swap(_size, obj._size);
    return *this;
}

Best of luck.

Answer 2

_size has no initial values, since you have not passed it either as arguements or initialized anywhere else before using it.

Also, don't do *_arr[i] = *obj._arr[i];

It doesn't make sense as arr[i] itself means *(arr+i).

Do:

_arr[i] = obj._arr[i];

Answer 3

when you do this, you should be fine

Array::Array( const Array & obj ):
    _size(obj._size) 
{
    _arr = new int[_size];

    for(int i=0;i<_size;i++)
        _arr[i] = obj._arr[i];
}

on line 2 you get the intializer list, read this http://www.cprogramming.com/tutorial/initialization-lists-c++.html

The point is, when you write a custom copy constructor, only the things you explicitly say you copy will be copied. The copy constructor will first call the default initilizer for all members except when you use the initializer value.

Also, a tiny hint for performance maybe, did you look into memcpy? http://www.cplusplus.com/reference/cstring/memcpy/ it might be more efficient :D

another thing I noticed is that you dereference *_arr[i], which is not a pointer but a reference to the value.

EDIT someone noticed that my even better was actually not better. I changed it.

Answer 4

_size being a primitive type is being copied is actually a true statement, but you didn't specify the complete sentence.
When you don't declare a copy constructor the default implementation will be something as this

Array::Array( const Array & obj )
{
    this._size = obj._size;
    this._arr = obj._arr;
}

Which is what you don't want as the copied object will share the _arr variable.
But as you have implemented the copy constructor you need to implement it fully. In your case you not not used the value of _size. Hence it would be undefined.

The compilation failure is due to the line *_arr[i] = *obj._arr[i];. You are using an illegal indirection using the *. Just use _arr[i] = obj._arr[i]; to copy it.

Answer 5

It's my solution. Maybe will help.

Array.h

#pragma once
class Array {

public:

    Array();
    Array(int size);
    Array(const Array& src);
    Array operator=(const Array& rhs);
    ~Array();

    void readInValues();
    void mult(int num);
    void add(int num);
    void printArray();

private:
    int *_arr;
    int _size;

};

I added the assignment operator in reference to the three rule. And it's Array.cpp

#include "Array.h"

Array::Array()
    :_size(10) {

    _arr = new int[_size];
}

Array::Array(int size)
    : _size(size) {

    _arr = new int[_size];
}

Array::Array(const Array& src) {
    _size = src._size;
    _arr = new int[_size];

    for (int i = 0; i < _size; i++) {
        _arr[i] = src._arr[i];
    }
}

Array Array::operator=(const Array& rhs) {
    if (this == &rhs) {
        return (*this);
    }

    delete[] _arr;

    _size = rhs._size;
    _arr = new int[_size];

    for (int i = 0; i < _size; i++) {
        _arr[i] = rhs._arr[i];
    }
}

Array::~Array() {
    delete[] _arr;
}