C++ rule of five for class that has dynamic memory

Question

So I'm writing the big five for a class that has dynamic int array

struct intSet {
  int *data;
  int size;
  int capacity;

  intSet();
  ~intSet();
  intSet(const intSet& is);
  intSet(intSet &&is);
  intSet &operator=(const intSet& is);
  intSet &operator=(intSet &&is);
}

What I got so far:

intSet::intSet(const intSet& is){
  this->size=is.size;
  this->capacity=is.capacity;
  this->data=is.data;
}

intSet::intSet(intSet &&is){
  this->size=is.size;
  this->capacity=is.capacity;
  this->data=is.data;
  is.data=nullptr;
}

intSet& intSet::operator=(const intSet& is){
  if(&is!=this){
    size=is.size;
    capacity=is.capacity;
    delete [] data;
    data=is.data;
    data=new int[capacity];
    for(int i=0;i<size;i++){
      data[i]=is.data[i];
    }  
  }
  return *this;
}

intSet& intSet::operator=(intSet &&is){
  if(&is!=this){
    size=is.size;
    capacity=is.size;
    delete [] data;
    data=is.data;
    is.data=nullptr;
  }
  return *this;
}

intSet::~intSet(){
  delete [] this->data;
}

Clearly there's something wrong with it but I am not really familiar with the big five...I searched a lot but still did not find out the answer...

You mean "Rule of five": http://en.cppreference.com/w/cpp/language/rule_of_three — , Jun 26 '16 at 22:47
Your copy constructor is making a shallow copy, yet your copy assignment operator is making a deep copy. Also, please use the constructor initializer-list, this isn't Java. — user2296177, Jun 26 '16 at 22:47
@BryanChen so the big five are copy constructor, copy assignment, move constructor, move assignment. — anddn, Jun 26 '16 at 22:48
if possible, use `shared_ptr` or `unique_ptr` and follow Rule of Zero — Bryan Chen, Jun 26 '16 at 22:52

eerorika · Answer 1 · 2016-06-27T00:28:31.200

Clearly there's something wrong with it ... did not find out the answer...

The biggest wrong is in the copy constructor.

When you simply copy the pointer, then both the copy and the original point to the same array. When one of them is destroyed, the destructor deletes the pointed array, at which point the pointer in the other object becomes invalid, and its use will have undefined behaviour.

Solution: Allocate a new array instead. In other words: do a deep copy, rather than shallow. If you need help figuring out how to do that, simply take a look at your copy assignment operator implementation (although, you can simplify with std::copy).

The copy assignment operator has flaws as well:

There is a redundant data=is.data;, which is pointless, since data is overwritten on the next line.
Once you have fixed the copy constructor to do a deep copy, like the assignment operator does, both of them will contain duplicated code for allocation of new array and copying contents. Having duplicate code is slightly bad.
The operator does not provide a strong exception guarantee. If allocating the new array throws an exception, the member pointer will be pointing to a deleted array (leading to UB). Even if the allocation succeeds, the copying of contents may result in an exception. If it does, then the partial copy is not rolled back, and the object remains in an inconsistent state. Lack of strong exception guarantee is moderately bad.

A solution to above problems is to use the popular copy-and-swap idiom to implement the copy assignment operator.

Better solution: From what little is shown, your class appears to be re-inventing std::vector. There is hardly ever a need to do that. Simply use std::vector instead.

C++ rule of five for class that has dynamic memory

1 Answers1