43

I have an Animal class with a virtual destructor, and a derived class Cat.

#include <iostream>

struct Animal
{
    Animal() { std::cout << "Animal constructor" << std::endl; }
    virtual ~Animal() { std::cout << "Animal destructor" << std::endl; }
};

struct Cat : public Animal
{
    Cat() { std::cout << "Cat constructor" << std::endl; }
    ~Cat() override { std::cout << "Cat destructor" << std::endl; }
};

int main()
{
    const Animal *j = new Cat[1];
    delete[] j;
}

This gives the output:

Animal constructor
Cat constructor
Animal destructor

I don't understand why is the Cat's destructor not called, when my base class destructor is virtual?

user3840170
  • 26,597
  • 4
  • 30
  • 62
suhovhan
  • 431
  • 2
  • 3
  • cannot reproduce https://godbolt.org/z/asahqsvxM. What compiler are you using? – 463035818_is_not_an_ai May 15 '23 at 11:23
  • which standard version are you using? Please only tag that and remove the other – 463035818_is_not_an_ai May 15 '23 at 11:23
  • 10
    Totally OT, but if a function doesn't have any arguments then you don't need to write anything. Instead of e.g. `Cat(void)` only `Cat()` will work. – Some programmer dude May 15 '23 at 11:24
  • Also OT, but the member should not be a raw pointer, and when it is you need to consider the rule of 3 / 5. – 463035818_is_not_an_ai May 15 '23 at 11:25
  • 1
    @463035818_is_not_a_number clang 14.0.3 on macOS reproduces OP's output for me. – Botje May 15 '23 at 11:26
  • 2
    @Botje I can only reproduce the output with gcc when `Animal::~Animal` is **not** virtual (https://godbolt.org/z/eKndcKGbM) – 463035818_is_not_an_ai May 15 '23 at 11:26
  • 1
    @463035818_is_not_a_number I changed your first compiler explorer link to x86-64 clang 16.0.0 and got OP's output. – Botje May 15 '23 at 11:28
  • silly bugs require silly thinking; maybe a `#define virtual` somewhere? Check with `#ifdef virtual \n #error "virtual is defined" \n #endif` – Raf May 15 '23 at 11:28
  • 1
    Minimal repro: https://godbolt.org/z/EaaYnKPMP Note the const isn't needed, but array new/delete is. – Mike Vine May 15 '23 at 11:34
  • 1
    @Raf no silly thinking required. We already found out that nothing but posted code is needed to reproduce the output with clang https://godbolt.org/z/aKxMnozbW. Only OP needs to clarify what they are using – 463035818_is_not_an_ai May 15 '23 at 11:34
  • 2
    (I've answered this, but interesting to note that GCC gets just as confused later if you change the number of cats allocated to _2_) – Mike Vine May 15 '23 at 12:03
  • 1
    You almost never want an array of polymorphic types. You want an array of (smart) *pointers* to such types instead. – n. m. could be an AI May 16 '23 at 04:54
  • 1
    Just to follow up on the `Cat(void)` vs. `Cat()` thing—in C, you should use the former (the `void` distinguishes between unspecified and empty arity), but in C++ it's an oxymoron (this cruft has been sensibly removed) and you should use the latter. Highly recommended reading: https://softwareengineering.stackexchange.com/a/287002 – geometrian May 16 '23 at 18:12
  • Since Cat and Animal are not the same size, you are going to have an issue if you try to index more than one Cat in the Animal array. This is also probably related to why some compilers are not immediately getting confused on this specific example. – penguin359 May 17 '23 at 07:30
  • Deleting through the converted pointer is UB, as one (correct) answer says. There is no guarantee in general that the converted pointer points to the beginning of the array. (in case of multiple base classes, for one) Storing the result of `new[]` in a variable declared with `const auto` should keep the **type** and **value** of `j` appropriate for use with `delete[]`, and the code would work as expected. It's (almost) no use trying to find a sensible reason for a specific behaviour in presence of UB. It resorts to compiler psychology, not logic. – Laurent LA RIZZA May 17 '23 at 12:22

7 Answers7

42

Note that whilst a Cat is an Animal, an array of Cats is not an array of Animals. In other words, arrays are invariant in C++, not covariant like they are in some other languages.

So you are up-casting this array and this later confuses the compiler. You must do array delete[] in this case on the correct, original, type - Cat*.

Note that you would have similar issues for the same reason if you allocated an array of 2 or more Cats, cast this to an Animal* and then tried to use the second or subsequent Animal.

Mike Vine
  • 9,468
  • 25
  • 44
  • 1
    Typo: down casting -> Upcasting (Downcasting would be Animal -> Cat) – Botje May 15 '23 at 11:45
  • 4
    `std::vector` to the rescue – 463035818_is_not_an_ai May 15 '23 at 11:53
  • Does a tool like CPPCheck see the issue? Can someone point a precise wording, perhaps in cppreference, about why, while storing a derived * as base * is legal, the unexpected delete [] is called? – Oersted May 15 '23 at 12:11
  • @Dharmesh946 I added such answer. – AnArrayOfFunctions May 16 '23 at 01:45
  • Could you elaborate on the terms *invariant* and *covariant*? I tried looking them up on a web search, but there was too much (off-diagonal) covariance with the math/data term. – Andrew Holmgren May 17 '23 at 12:23
  • 1
    @AndrewHolmgren https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science) – n. m. could be an AI May 17 '23 at 13:25
  • 3
    This would become even clearer if you had a data member in `Animal` and added a second data member to `Cat`. Since then `sizeof(Animal)!=sizeof(Cat)`, array indexing would be completely off. An array of *smart pointers* to `Animal` would work as expected since the destructor would get called for each pointer, and pointers are genuinely polymorphic. – Mark Ransom May 18 '23 at 02:31
  • @463035818_is_not_a_number It's not even an array thing. In C++, a `Cat` is not an `Animal`, full stop. C++ simply does not fully implement (I want to say: "believe in") the OOP is-a relationship, not in the way e.g. Java does. We can use the is-a model to design/plan class hierarchies. But the correct mental model for actually writing C++ is that a `Cat` only *contains* a (special) `Animal` object, as a base-class subobject. `std::vector` doesn't help much: a `std::vector` cannot hold `Cat`s. Assigning a `std::vector` to one makes a copy of only the `Animal` part of each `Cat`. – HTNW May 18 '23 at 07:30
  • @HTNW what you say all makes sense, but frankly, has nothing to do with my confusion. A `std::vector` would be fine with a cat. Its obvious that a `std::vector` is not a vector of pointers to `Animal`, but as soons as c-arrays are involved I get confused – 463035818_is_not_an_ai May 18 '23 at 07:34
10

It's Undefined Behaviour in my understanding because of (in 7.6.2.9 Delete,p2, Emphasis mine):

In a single-object delete expression, the value of the operand of delete may be a null pointer value, a pointer value that resulted from a previous non-array new-expression, or a pointer to a base class subobject of an object created by such a new-expression. If not, the behavior is undefined. In an array delete expression, the value of the operand of delete may be a null pointer value or a pointer value that resulted from a previous array new-expression...

Which basically means that for delete[] the type must be the exact one from new[] (no base class sub-objects allowed like delete).

So for the reason that is like this - in my opinion this time is obvious - the implementation needs to know how much is the full object size so it can iterate to the next array element.

I wrote counter arguments for why this could be different - but after some thought (and reading the comments) - I realised that such a solution is solving X-Y problem.

AnArrayOfFunctions
  • 3,452
  • 2
  • 29
  • 66
  • 2
    This encourages dangerous behaviour though. If you allow this then you are basically saying `Animal* a = new Cat[2]; delete[] a;` is ok. This would implicitly imply that `a` is useful on its own when it very much is not. `a[1]` is undefined behaviour at this point as array index uses the type (`a[i]` is defined as `(a+i)`) which moves on `a` by `sizeof(Animal)` bytes and _not_ `sizeof(Cat)` bytes which it must do. So just avoiding this whole issue by not allowing `a` to be useful across the board is probably the right thing to do. – Mike Vine May 16 '23 at 11:07
  • Re "the implementation needs to store the number of array elements anyway": The number of elements may be found dynamically by an end marker. The destructor iterates the array *anyway* if element destructors are called, so that's not a time loss. The allocated block will probably store a block size, but that may not be the minimum required for the array size (it could always be a power of 2, for example). If the element type does *not* have a destructor, then the end marker is irrelevant and no iteration at all will take place -- the block can be released right away wholesale. – Peter - Reinstate Monica May 17 '23 at 07:17
  • In addition, the first assignment in `main` does not guarantee in general that the `Animal*`, pointing to the first element of the array as an `Animal` would have the same **value** as the pointer returned by `new Cat[]`, especially if `Animal` is not the first base class of `Cat` and EBO did not kick in on the previous ones. So yes, in general, deleting through the converted pointer ought to be UB. Using `const auto` instead should fix it :-P – Laurent LA RIZZA May 17 '23 at 12:04
9

I answer to my own comment: https://en.cppreference.com/w/cpp/language/delete (emphasis mine)

For the second (array) form, expression must be a null pointer value or a pointer value previously obtained by an array form of new-expression whose allocation function was not a non-allocating form (i.e. overload (10)). The pointed-to type of expression must be similar to the element type of the array object. If expression is anything else, including if it's a pointer obtained by the non-array form of new-expression, the behavior is undefined.

https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing

Informally, two types are similar if, ignoring top-level cv-qualification:

  • they are the same type; or
  • they are both pointers, and the pointed-to types are similar;or
  • they are both pointers to member of the same class, and the types of the pointed-to members are similar; or
  • they are both arrays of the same size or both arrays of unknown bound, and the array element types are similar. (until C++20)
  • they are both arrays of the same size or at least one of them is array of unknown bound, and the array element types are similar.

So far as I understand, inheritance is not similarity...

Oersted
  • 769
  • 16
  • 5
    Inheritance cannot be similarity indeed. Note that the array contains values, not references, and a `Cat` values is larger than an `Animal` value, so it won't fit there. That makes the cast from `Cat *` to `Animal *` invalid *when pointing to an array*, but since the type does not indicate it's pointing to an array and for single instance it is valid, the programmer has to mind this. – Jan Hudec May 16 '23 at 06:07
  • 3
    ah yes - note that if you allocate more than one cat then for example `j[1]` calculates a wrong address for the second cat, because it uses sizeof(Animal) instead of sizeof(Cat) – user253751 May 16 '23 at 15:01
  • Where do they get the "similarity" idea from? The standard only seems to say "pointer value that resulted from a previous array new-expression". Perhaps there is a constraint or additional remark somewhere which is not cross-referenced from 7.6.2.9? By the way, I must assume that "value" in the standard means "typed value" (and not simply numerical value, or the OP's code would work). But the standard does not define its use of "value" afaics, which may be a defect. – Peter - Reinstate Monica May 17 '23 at 07:46
  • @Peter-ReinstateMonica it came from http://wg21.link/cwg2474 – Cubbi Jun 01 '23 at 21:08
  • @Cubbi Interesting, thanks. As an aside, the definition of "similar" is unintelligible to me. (I assume they mean roughly that cv-qualifications don't matter and that one can omit array bounds, but that's only a qualified guess. Examples would really help here.) – Peter - Reinstate Monica Jun 01 '23 at 23:07
6

My only criticism of the other answers is that they are too tame. Arrays work because the entries all have the same size, so the starting address of the entry with index 5 can be computed easily. Casting array of Derived to array of Base (which is what you are doing here) could have much worse consequences than you have shown. (If the array happens to have only one entry or Derived has no new data members you might be OK.) A[1].method() will probably do something strange because (Base*)(A)+1 is not even the starting address of any object. The method will read the wrong data, and the effect of any change will be highly unpredictable. Any call to a virtual function will use an entirely meaningless vtable pointer, so who knows what 'code' will be executed. Even if that call is not an immediate disaster, you might override a vtable pointer. Destruction of the array will almost certainly delete some meaningless 'pointers'.

Pointer to single object (allocated & deleted without '[]') is fine. You can cast to Base*, dynamic_cast back, and call virtual functions. That's why Ayxan Haqverdili's suggestion works. Array of X should never have anything except objects of run-time type X, and new[] (even new[1]) creates arrays.

2

Others have explained the issue, but if you want an array of polymorphic objects, you want an array of pointers. Here's one way:

vector<unique_ptr<Animal>> animals;
animals.push_back(std::make_unique<Cat>());
animals.push_back(std::make_unique<Dog>());
animals.push_back(std::make_unique<Monkey>());
Aykhan Hagverdili
  • 28,141
  • 6
  • 41
  • 93
0

The Cat's destructor is not called because the pointer J is declared as pointer to a constant Animal. When you delete an array of objects through a pointer to a base class with a virtual destructor the destructor of each derived class object is not called unless the pointer type is of derived class.

So for calling the Cat Destructor. modity the pointer type to

int main()
{
const Cat* j=new Cat[1];
delete[] j;
}
Zarak.K
  • 1
  • 3
-3

Make the Cat destructor also virtual and the code works as expected.

  • 1
    As the destructor of the base class (Animal) is declared virtual, the destructor of the subclass Cat will implicitly be virtual too, as virtual-ness is propagated. – Daniel Hedberg May 17 '23 at 21:44
  • 1
    Please read attentively all answers and comments, as the issue is tricky. You'll see that it may "work" but not on all compilers and not in all situations, and rightly so. – Oersted May 18 '23 at 11:10