Dynamic array deallocation (assignment operator vs. copy constructor)

Question

Should i deallocate dynamic array (allocated in constructor) in copy constructor and/or assignment operator?

struct Test
{
  const size_t n;
  int* xs;

  Test(const size_t n)
    : n(n)
    , xs(new int[n])
  { }

  Test(const Test& other)
    : n(other.n)
    , xs(new int[n])
  {
    memcpy(xs, other.xs, n * sizeof(int));
  }

  Test& operator=(const Test& other)
  {
    n = other.n;
    delete[] xs;
    xs = new int[n];
    memcpy(xs, other.xs, n * sizeof(int));
  }

  ~Test()
  {
    delete[] xs;
  }
};

void main()
{
  Test a(10);
  Test b(a);
  Test c(20);
  c = b;
}

As you can see, I guess that you have to delete[] the array in assignment operator implementation (since it has been already allocated somewhere during construction of the object that's being assigned to). And I do think that you don't need to deallocate the array while copy constructing the object, since it has not been constructed yet.

The thing is, running the application above under Application Verifier shows no memory leaks no matter if there is delete[] in operator= or if there is not. Application runs OK though in both cases.

So, should I delete[] xs in copy constructor, assignment operator, both or neither?

Test& operator=(const Test& other) : n(other.n) compiled? And yes, you should. — Luchian Grigore
@LuchianGrigore no, I've typed it w/o compiling (I don't have a compiler on the computer I'm browsing SO from). Thanks for noticing! — Egor Tensin
@LuchianGrigore But why Application Verifier shows no memory leaks in both cases? Isn't it a trustworthy tool? — Egor Tensin
You shouldn't use new[] and delete[] at all. Use a std::vector. — jamesdlin
@jamesdlin I know, but I can't. I'm striving for performance (I actually have hundreds of thousands of similar structures allocated and copied constantly). I've tried std::vector and it turned out to be awfully slow. — Egor Tensin

Andy Prowl Andy Prowl · Accepted Answer · 2013-03-22T10:27:51

In general, manual memory management is a bad idea. You shouldn't do it, and you should prefer std::vector<> or other container classes, unless you have a good reason for doing otherwise. This said...

So, should I delete[] xs in copy constructor, assignment operator, both or neither?

You should delete[] an array allocated with new[] before you lose the last pointer to that array. Otherwise, you will leak.

In practice, since the array you want to delete is owned and encapsulated by your class, this means that you will have to delete[] it in the assignment operator overload (before the pointer gets assigned to a new array, thus losing the only existing reference to the previous one) and in the destructor (when you are disposing of the object and then losing the only existing reference to the encapsulated array).

As you say, the copy constructor is a construction routine and the pointer wasn't previously referencing any allocated resource (in fact, there is no "previously" here, the object's lifetime hasn't even begun): therefore, it is not only not needed to delete[] here, it is wrong.

Also notice, that in order to avoid dangling pointers and possible undefined behavior, you should make sure that your class is indeed the only owner of the encapsulated array. Otherwise, code external to the class could delete[] it, or save pointers to it that will become dangling. Thus, you should not make the xs data member public.

Dynamic array deallocation (assignment operator vs. copy constructor)

1 Answers