C++ Dynamic Allocation Mismatch: Is this problematic?

Question

I have been assigned to work on some legacy C++ code in MFC. One of the things I am finding all over the place are allocations like the following:

struct Point
{
   float x,y,z;
};

...

void someFunc( void )
{
   int numPoints = ...;
   Point* pArray = (Point*)new BYTE[ numPoints * sizeof(Point) ];
   ...
   //do some stuff with points
   ...
   delete [] pArray;
}

I realize that this code is atrociously wrong on so many levels (C-style cast, using new like malloc, confusing, etc). I also realize that if Point had defined a constructor it would not be called and weird things would happen at delete [] if a destructor had been defined.

Question: I am in the process of fixing these occurrences wherever they appear as a matter of course. However, I have never seen anything like this before and it has got me wondering. Does this code have the potential to cause memory leaks/corruption as it stands currently (no constructor/destructor, but with pointer type mismatch) or is it safe as long as the array just contains structs/primitive types?

Answer 1

As long as the constructors and destructors do nothing, then you're safe.

Answer 2

Formally the code causes undefined behavior because of the pointer type mismatch in new[]/delete[]. In practice it should work fine.

The pointer type mismatch issue can easily be fixed by adding a cast to the delete-expression

delete [] (BYTE *) pArray;

If Point type is defined as shown in the question (i.e. with trivial constructor and destructor), then this correction solves all formal issues there are in this code. From the language point of view, the lifetime of an object with trivial constructor (destructor) begins (ends) simultaneously with its storage duration. I.e. there's no requirement to perform the actual invocation of constructor (destructor).

Answer 3

As long as you assure that it really does invoke a matching delete[] for every new[], it shouldn't leak -- but if an exception might be thrown by any of the code that's been commented out, that's going to be difficult to assure (basically, you need to catch any possible exceptions, delete the memory, then re-throw the exception).

Answer 4

I would first try to figure out why the code was written that way in the first place. It might be simply because the programmer didn't know any better, or because they were trying to work around some funky defect int he compiler. But there might be a real reason that you are unaware of. If there is, then unless you understand that reason and its side effects, you may introduce a defect by changing this code.

That out of the way, and assuming there is no particular reason why the code needs to be this way now, you should be safe in changing the code to use more modern and correct constructs.

But why? I understand the motivation to make the code more correct. But what do you really gain by this? If the code works the way it is now (a big assumption), then by changing the code you possibly gain the benefit of making the code more understandable to future programmers, but every line of code you change introduces the possibility for a new bug to be written.

And if finally you do decide to go ahead with the change, why stop halfway? Consider getting rid of all the news and deletes altogether, and replace them with vectors etc.