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, Mar 22 '13 at 10:23
@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, Mar 22 '13 at 10:26
@LuchianGrigore But why Application Verifier shows no memory leaks in both cases? Isn't it a trustworthy tool? — Egor Tensin, Mar 22 '13 at 10:27
You shouldn't use `new[]` and `delete[]` at all. Use a `std::vector`. — jamesdlin, Mar 22 '13 at 10:27
@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, Mar 22 '13 at 10:29
Using a `std::vector` shouldn't be significantly slower. What are you doing? How are you testing? http://stackoverflow.com/questions/381621/using-arrays-or-stdvectors-in-c-whats-the-performance-gap — jamesdlin, Mar 22 '13 at 10:32
@jamesdlin this is a really bad piece of advice. Without knowing the context it's very brash to make such statements. — SomeWittyUsername, Mar 22 '13 at 10:34
@jamesdlin I'm processing a trace file (hundreds of MBs) entirely consisting of small 64B structures (each has 4-element array inside). Array length might vary between different traces, so I'm sticking to a general solution with dynamic arrays. Processing such a trace file (I perform some pretty heavy computations) is not very fast by itself, and introducing ``std::vector`` makes it quite hard to wait until it finishes (no precise measurements were done though). — Egor Tensin, Mar 22 '13 at 10:36
@icepack - Disagree. std::vector (used properly) is VERY unlikely to be significantly worse than the code we're seeing here. However, if you really want to avoid std::vector, you should at least use `boost::scoped_array` to manage the pointer. — Roddy, Mar 22 '13 at 10:37
@Roddy 1. "*Used properly*"? So you're replacing one problem with the other. And OPs comment above demonstrates the problematics clearly. 2. What is *significantly worse* to you? Maybe it's not the same for OP. 2. Binary size? What if there are severe limitations on it (firmware etc.)? 3. Availability of std libraries? Not in every environment. 4. There are reasons for native arrays in C++, one of which is that C++ isn't always about pushing to higher level abstractions, it's a general purpose language — SomeWittyUsername, Mar 22 '13 at 10:39
@Roddy Hey, it might be a good idea since I'm using boost anyway! I'll give it a try. — Egor Tensin, Mar 22 '13 at 10:39
@EgorTensin - For your solution to work you need to know array size before you create the `Test` object. With `std::vector`, using `std::vector.resize()` will avoid the vector continually resizing/copying as it grows. Were you doing that? — Roddy, Mar 22 '13 at 10:40
@EgorTensin: Since you're using Windows, did you also make sure that you disabled `_HAS_ITERATOR_DEBUGGING` (and maybe `_SECURE_SCL`)? — jamesdlin, Mar 22 '13 at 10:41
One other thing : Your assignment operator is broken : It creates an array of the same size as the source, but doesn't copy the array contents from the source. Maybe that's why it's faster than `std::vector`? — Roddy, Mar 22 '13 at 10:45
@Roddy No, it's just because I was too lazy to add ``memcpy``. Thanks for noticing! — Egor Tensin, Mar 22 '13 at 10:51
@Roddy Yes, and performance is still very bad using ``std::vector.resize()``. — Egor Tensin, Mar 22 '13 at 10:52
@EgorTensin, also your assignment operator doesn't check for assign to self, which is often a bug waiting to happen... consider `if (&other == this)` and what happens to the arrays... — Roddy, Mar 22 '13 at 10:55
@EgorTensin not sure how do you use your class but if your compiler supports C++11 you can avoid at least part of the data copying overhead by using move semantics — SomeWittyUsername, Mar 22 '13 at 11:12
@EgorTensin Search for `_HAS_ITERATOR_DEBUGGING` and `_SECURE_SCL`... — jamesdlin, Mar 22 '13 at 17:08

Andy Prowl · Accepted Answer · 2013-03-22T10:39:10.083

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.

Thank you, I'm sticking with that from now on. I was worried about what Application Verifier shows, but your answer obviously makes sense to me so screw it. — Egor Tensin, Mar 22 '13 at 10:32
@EgorTensin: Yes, I'd say screw it as well. Or maybe post a specific question asking what is it needed to do to make Application Verifier detect leaks. I do not know that tool unfortunately so I can't help with that. — Andy Prowl, Mar 22 '13 at 10:34
Considering your ``std::vector`` advice you are welcome to review comments to the question (in case you're interested). — Egor Tensin, Mar 22 '13 at 10:38
@EgorTensin: Yes, I did, I just wrote my recommendation before you clarified things. — Andy Prowl, Mar 22 '13 at 10:44

Dynamic array deallocation (assignment operator vs. copy constructor)

1 Answers1