Exercising in copy constructors and assignment operators

Question

I've just created an implementation of array to train myself in copy constructors, assignment operators, and exception safety. Please look at this code and tell me what is wrong here.

class DumbArray {
public:
 DumbArray(size_t sz)
 : _size(sz), _buf(new int[_size])
 {
 memset(_buf, 0, _size * sizeof(int));
 }
 DumbArray(const DumbArray& arr)
 : DumbArray(arr._size)
 {
 int* newMem = new int[arr._size];
 if(newMem) {
 std::copy(arr._buf, arr._buf + arr._size, newMem);
 delete [] _buf;
 _buf = newMem;
 _size = arr._size;
 }
 }
 DumbArray(DumbArray&& arr) {
 swap(arr);
 }
 DumbArray(std::initializer_list<int> ilist) 
 : DumbArray(ilist.size())
 {
 std::copy(ilist.begin(), ilist.end(), _buf);
 }
 DumbArray& operator=(const DumbArray& arr) {
 if(this != &arr) {
 DumbArray tmp(arr);
 swap(tmp);
 }
 return *this;
 }
 DumbArray& operator=(DumbArray&& arr) {
 swap(arr);
 return *this;
 }
 ~DumbArray() {
 // _size = 0; // should this be done? object is being destroyed anyway, yup?
 delete [] _buf;
 }
 void swap(DumbArray& arr) {
 std::swap(_buf, arr._buf);
 std::swap(_size, arr._size);
 }
 int& operator[](int idx) {
 if(idx < 0 || idx >= _size) {
 throw std::out_of_range("Incorrect index");
 }
 return _buf[idx];
 }
 size_t size() const {
 return _size;
 }
private:
 size_t _size;
 int* _buf;
};

Answer 1

This is reasonably clean and reasonably complete code but I see some things that might improve your code.

Consider making this a template

There is no reason that this data structure should be limited to just storing int values. Consider reworking it as a templated class to make it more general and to get additional practice.

Implement member functions non-inline

All of those inline functions can increase the size of your code. If you declare them first and then have out-of-line implementations(at least for the non-trivial functions), it has advantages, especially if you decide to make this a templated class.

Consider making the destructor virtual

By having a concrete destructor rather than a virtual one, you are effectively prohibiting derived classes. If that's not your intent, making your destructor virtual will make it possible to derive classes from this one.

List the required #include files

By my reckoning, this code requires the following to work:

#include <cstring>
#include <initializer_list>
#include <iterator>
#include <stdexcept>

These are therefore an essential part of the class interface.

Reconsider the use of size_t

Although it's used often in the C standard library, you don't really need it here, and requiring a C-flavored include file doesn't help much in keeping the interface small. You could use unsigned instead and reduce the number of required include files.

Avoid comparing signed to unsigned

In your operator[], idx is declared as int but _size is size_t so the range check at the top of the function compares signed and unsigned numbers without a cast. Since negative numbers are already eliminated by the first clause, so you could use either a C-style cast (gasp!) or a reinterpret_cast on idx in the second clause.

Add a default constructor

Because DumbArray has no default constructor (one with no arguments), code like this:

std::vector<DumbArray> dv(10); // make ten arrays

cannot compile or run. You could either add a default value for your existing constructor or create a new default constructor.

Consider using std::fill_n instead of memset

Instead of using the old-school

memset(_buf, 0, _size * sizeof(int));

you might consider instead using fill_n:

std::fill_n(_buf, _size, 0);

since you're already using the similar std::copy in one of the constructors.

Add a const version of operator[]

As it is currently written, there's no way to index into a const DumbArray. Adding const to operator[] will fix that problem but make sure it returns a copy of an int rather than a reference to one.

• edmz: Note: std::fill_n and (std::)memset do not do the exact same thing. I wanted to point that out, although your advice is absolutely right.
• vortexxx192: But still got a question: why should I declare initializer_list as const, even if it is being passed by value? Who cares then?
• Edward: @vortexxx192: good question. I actually tried to come up with a case in which it might matter to have the initializer_list as const and came up with nothing. The more I think about it, the less I like my own advice. I'm going to delete it from my answer.
• TemplateRex: most of your tips are pretty sound, but the virtual destructor sounds iffy. I would go the opposite way and make the entire class final! Furthermore, I would incorporate unique_ptr in lieu of raw new and delete. For one, this would greatly improve exception safety (on which you are completely silent).
• dyp: "Consider using std::fill_n instead of memset" Consider using neither. new int[_size]() will do the same.

Answer 2

A bug

DumbArray(DumbArray&& arr) {
 swap(arr);
}

You always have to make sure to leave the moved-from object in a state that's at least safe to destroy. Per convention, better leave it in a state where you use any member function safely that has no precondition, like operator=, swap, or size() in your case. You can even make sure your checked operator[] is always well-behaved.

The constructor does not initialize this->_buf, so arr._buf will have that "uninitialized" (indeterminate) value after swapping. But arr will eventually be destroyed, which calls delete _buf using that indeterminate value. This causes Undefined Behaviour. => Bad

Example:

DumbArray a(10); // `a` is properly initialized
DumbArray b( std::move(a) ); // swaps properly initialized `a` with uninitialized `b`
// `a` is now uninitialized

This is of course worse if a is a temporary:

DumbArray b( DumbArray(10) ); // destroys a temporary with an indeterminate `_buf`

Using C++11 library features

As TemplateRex already noted, std::unique_ptr is a helper class that's immensely useful to build resource manager classes (RAII classes). You already use constructor delegation, yet there are still two places where you call new and two places where you call delete. (Actually, one is really redundant, see below.)

You can further reduce code duplication by using the same code for copying from an initializer list and copying from another DumbArray (use pointer parameters).

About initializing dynamically allocated memory

Neither memset nor std::fill_n is necessary. new int[_size]() with parentheses will value-initialize, i.e. zero, all the ints.

Testing a newly allocated pointer

int* newMem = new int[arr._size];
if(newMem)

If you don't use weird compiler options, either rely on exceptions or on checking the pointer here. Per default, new will throw a bad_alloc exception when memory allocation fails (there are new handlers and such, but that's the default behaviour). If you don't want exceptions, use new(std::nothrow) from <memory>. My advice here: just drop the check.

Self-assignment

if(this != &arr) {
 DumbArray tmp(arr);
 swap(tmp);
}

The copy-and-swap idiom is self-assignment-safe. You don't have to check; in fact, the check is only an optimization in the rare cases where self-assignment actually happens. Otherwise, it's one more branch that could be mispredicted.

Efficiency

The copy-and-swap technique is great because it's simple and provides the strong exception safety guarantee. However, it prevents you from optimizing assignment for the case this->_size >= arr._size. Further optimizations can be gained by using two variables, capacity and current_size.

Additionally, you always zero the memory via constructor delegation. This is not necessary when copying, for example in the initializer-list-constructor. The copy constructor looks pretty weird in this respect: you allocate one buffer via constructor delegation, zero it, and replace it with a new buffer.

Converting constructors

If you have a constructor that converts from an abundant type like size_t which additionally is a fundamental type with many implicit standard conversions, you should make that constructor explicit. This can catch a lot of mistakes at compile-time and sometimes helps with overload resolution.

• Edward: Can you elaborate on the first point? It seems to me that member function swap initializes _buf. Can you provide sample code that shows the flaw?
• dyp: @Edward Ok, added an example. Hope that helps.
• vortexxx192: Can't understand an example. Why must something be swapped? Isn't a copy constructor (that seems to be weird, yes, but correct) invoked in that case?
• Edward: @vortexxx192: the example isn't correct as written, but the idea is right. A correct example might be DumbArray a(func()); where func() returns a DumbArray. The problem manifests when the temporary is destroyed and calls delete[] _buf on an uninitialized _buf.
• dyp: @vortexxx192 m( Of course, my bad, sorry. Fixed the example.