99
votes

I went to a job interview today and was given this interesting question.

Besides the memory leak and the fact there is no virtual dtor, why does this code crash?

#include <iostream>

//besides the obvious mem leak, why does this code crash?

class Shape
{
public:
    virtual void draw() const = 0;
};

class Circle : public Shape
{
public:
    virtual void draw() const { }

    int radius;
};

class Rectangle : public Shape
{
public:
    virtual void draw() const { }

    int height;
    int width;
};

int main()
{
    Shape * shapes = new Rectangle[10];
    for (int i = 0; i < 10; ++i)
        shapes[i].draw();
}
3
Besides the missing semicolon, you mean? (That would be a compile-time error, though, not runtime)Platinum Azure
Are you sure they were all virtual ?Yochai Timmer
It Should be Shape ** It's pointing to an array of Rectangles. Then the access should have been shapes[i]->draw();RedX
@Tony good luck then, keep us informed :)Seth Carnegie
@AndreyT: The code is correct now (and was also correct originally). The -> was a mistake made by an editor.R. Martinho Fernandes

3 Answers

150
votes

You cannot index like that. You have allocated an array of Rectangles and stored a pointer to the first in shapes. When you do shapes[1] you're dereferencing (shapes + 1). This will not give you a pointer to the next Rectangle, but a pointer to what would be the next Shape in a presumed array of Shape. Of course, this is undefined behaviour. In your case, you're being lucky and getting a crash.

Using a pointer to Rectangle makes the indexing work correctly.

int main()
{
   Rectangle * shapes = new Rectangle[10];
   for (int i = 0; i < 10; ++i) shapes[i].draw();
}

If you want to have different kinds of Shapes in the array and use them polymorphically you need an array of pointers to Shape.

38
votes

As Martinho Fernandes said, the indexing is wrong. If you wanted instead to store an array of Shapes, you would have to do so using an array of Shape *'s, like so:

int main()
{
   Shape ** shapes = new Shape*[10];
   for (int i = 0; i < 10; ++i) shapes[i] = new Rectangle;
   for (int i = 0; i < 10; ++i) shapes[i]->draw();
}

Note that you have to do an extra step of initializing the Rectangle, since initializing the array only sets up the pointers, and not the objects themselves.

13
votes

When indexing a pointer, the compiler will add the appropriate amount based on the size of what's located inside the array. So say that sizeof(Shape) = 4 (as it has no member variables). But sizeof(Rectangle) = 12 (exact numbers are likely wrong).

So when you index starting at say... 0x0 for the first element, then when you try to access the 10th element you're trying to go to an invalid address or a location that's not the beginning of the object.