1

Related to the program from 2004 that I'm fixing. The former developer used the following code to free 'len' elements of an array in the destructor:

unsigned int* _data;  
...  
if (_data) {
  int len = size();
  delete (unsigned int[len]) _data;
}

I can't compile this code with my compiler. The error message is:

error: ISO C++ forbids casting to an array type ‘unsigned int [(((unsigned int)(((int)l) + -0x00000000000000001)) + 1)]’

There must be a reason he didn't use delete _data;How should I fix this error?
Thanks.

Community
  • 1
  • 1
sean
  • 1,632
  • 2
  • 15
  • 34
  • Probably by not doing this. This is almost certainly invoking undefined behaviour. – Oliver Charlesworth Sep 11 '13 at 22:39
  • 4
    That code was meaningless then, and it is meaningless now. Your compiler is just better at catching this atrocity. Just do `delete[] _data`. – Igor Tandetnik Sep 11 '13 at 22:40
  • 2
    By not using a manual dynamic allocation in the first place; use a `std::vector`. And if you want it manually cleared, then do that: `clear()`. – WhozCraig Sep 11 '13 at 22:40
  • 4
    The reason could be ignorance. Does the rest of the code make you think he knew what he was doing? – john Sep 11 '13 at 22:42
  • 4
    with due respect to the comments above, don't try to suggest don't use arrays, use vectors etc., when it's mentioned that the code is from an application written in 2004 and no one touched it till now. In most companies, it's not acceptable to make changes that are not in scope. If that's not the case in yours then I guess you are working on something that's not so critical in terms of business. – Jagannath Sep 11 '13 at 22:44
  • 3
    @Jagannath: If your company prevents you from making those changes, you are free to ignore the advice. For everyone else reading the question, the suggestion is useful. – Benjamin Lindley Sep 11 '13 at 22:58

2 Answers2

5

How should I fix this error?

Remove the cast, and look for the place where _data has been allocated.

  • If it has been allocated as new [someLength], replace with delete[] _data;
  • Otherwise (although that would be unlikely) replace with delete _data.

In the long run it is best prefer using dynamic containers, such as std::vector<unsigned int>, to dynamic allocations of arrays of primitives. I understand that this may be beyond the scope of your current refactoring, though.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Although in this case since _data is just a pointer to int, doing delete[] is no different than delete since there are no destructors to call. – Mike Makuch Sep 11 '13 at 22:50
  • 2
    @koodawg: It's different in the sense that it is correct, and doesn't invoke undefined behavior. Pretty important difference, imo. – Benjamin Lindley Sep 11 '13 at 22:53
2

First of all, check if you dynamically allocate _data somewhere in your code. If there isn't a new expression, you mustn't use delete.

If it is dynamically allocated using new, the moment you need to release that object you should ask yourself: "Is this pointer pointing to a single object, or is it pointing to an array of objects?". This is very important because the memory layout is different in each case and delete must know in advance how many destructors it should call. If it calls the wrong number of destructors, undefined behavior happens.

The rule of thumb is to use delete [] if and only if you allocated that object with [] in the new expression.

Daniel Martín
  • 7,815
  • 1
  • 29
  • 34
  • "the memory layout is different in each case" - is it? – Oliver Charlesworth Sep 11 '13 at 23:05
  • 1
    memory layout is implementation-defined. It could allocate one huge block and cram it all together, it could leave space between elements (similar to struct packing), it could allocate each array element separately. Who knows? In practice, it often is slightly different however. –  Sep 11 '13 at 23:16
  • @OliCharlesworth It depends on the implementation, but it usually is. – Daniel Martín Sep 11 '13 at 23:19
  • @DanielMartín: I appreciate that the standard allows for this (and so I completely agree that `delete` vs. `delete[]` is important), but I was under the impression that `new` and `new[]` essentially both end up calling `malloc` under the hood in most implementations. – Oliver Charlesworth Sep 11 '13 at 23:38
  • From a theoretical perspective, it could leave gaps between elements. For example, perhaps an array of bytes has each element aligned on word boundaries (e.g. 4/8 bytes) for efficiency of access. In practice this rarely if ever happens. My point is the standard leaves this up to the implementation. –  Sep 13 '13 at 01:19
  • @JohnGaughan: If it did that, then `sizeof(byte)` would need to be 4/8, because otherwise pointer arithmetic would not work. – Oliver Charlesworth Sep 13 '13 at 20:34