Pointer to object arrays as members overwrite memory

Question

Here is the deal. We have 2 different classes Class F and Class O

class F {
    private:
        int x;
        int y; 
    public:
        int getXf(){ return x; }
        int getYf(){ return y; }
        f(int ,int);
};

class O {
    private:
        int n;
        int k;
        int x;
        int y;
        char type;
        int id;
        int t;
    public:
        O(int ,int ,int ,int ,int);
        int getX(){ return x; }
        int getY(){ return y; }
};

And we have a third class P, where we initialize the values. In the class we are creating the two arrays of objects.

class Prog {                                                                   
    public:
        int count;
        int fcount;
        O *o[]; //here we are declaring the arrays of objects
        F *f[];           
    public :
        //void init(); Here is the function where we initializing the values
};

Now the 2 for statements where we are creating the objects.

for(int i=0;i<10;i++){
        randx = rand() % 10;
        randy = rand() % 20;

        o[i] = new O(100,50,i,randx,randy);
    }


    for(int i=0;i<3;i++){
        randx = rand() % 10;
        randy = rand() % 10;

        f[i] = new F(randx, randy);
    }

When we are printing all of the objects are here but the first 3 of the first class are replaced by the objects of the seconds. Exactly the 100 and 50 (1st for) from randx and randy (2nd for) respectively.

it's not your question, but did you applied a seed for your random function? srand ( time(NULL) ); — Laurence, Jan 10 '13 at 14:41
When asking a question on StackOverflow, it is most helpful if you post your *actual* code. For instance, here you have the `F` constructor as a lower-case `f` - but it is unlikely that is the case in the actual code you compiled and ran. We can help you better if you post the real code. — Borealid, Jan 10 '13 at 14:41
Is the array really declared as `O *o[];` (no size), or `O *o[10];` which is needed for your code? — Bo Persson, Jan 10 '13 at 14:42
Can you place your Prog constructor in the question? Edit: nvm, you don't have one. You need to initialize the o and f. — Laurence, Jan 10 '13 at 14:42
@BoPersson Yes it is declared like O *o[]; because i need to change the number of the created objects. — Siakon, Jan 10 '13 at 15:08
@Siakon - That's not really possible, you cannot have arrays of unknown size. I ask because in the init code you assume there are 10 elements, but there are really none. That's likely why the elements overwrite each other. See the answers by Mike Seymour and pmr for more info. — Bo Persson, Jan 10 '13 at 15:13

score 6 · Answer 1 · edited May 23 '17 at 12:18

O *o[];

This declares an array of unknown size, which is an incomplete type. C++ doesn't allow that to be used as a class member, although some compilers will allow it as an extension, interpreting it as an array of zero size. In either case, it's not what you want.

If you know the array bound at compile time, then you should specify it:

O *o[10];

otherwise, you'll need to dynamically allocate an array at run time:

std::vector<O*> o;

for(int i=0;i<10;i++){
    randx = rand() % 10;
    randy = rand() % 20;

    o.push_back(new O(100,50,i,randx,randy));
}

I would also suggest storing objects, or possibly smart pointers, rather than raw pointers in the array. If you really do want raw pointers for some reason, then remember to delete the objects once you've finished with them since that won't happen automatically, and don't forget the Rule of Three.

Because of the explanation of what this does, that it is non-standard and good advice on how to proceed.. I wish I could like this answer multiple times. — Agentlien, Jan 10 '13 at 15:09

score 5 · Answer 2 · answered Jan 10 '13 at 14:40

You are declaring arrays, but you never allocate memory for them. What you are seeing is just how your code is walking all over the stack.

Something more appropriate:

struct X {}; struct Y {};

class P {
public:
  P() : xs(new X*[10]), ys(new Y*[10]) { init(); }

  ~P() {
    // delete all objects
    for(std::size_t i = 0; i < 10; ++i)
      delete xs[i];
    for(std::size_t i = 0; i < 10; ++i)
      delete ys[i];

    delete[] xs;
    delete[] ys;
  }
private:
  void init() {
    // initialize
    for(std::size_t i = 0; i < 10; ++i)
      xs[i] = new X();
    for(std::size_t i = 0; i < 10; ++i)
      ys[i] = new Y();
  }

  // prevent assignment and copy
  P& operator=(const P& other);
  P(const P&);

  X** xs;
  Y** ys;
};

Of course, all this magic becomes unnecessary if you just use std::vector to store your data.

score 1 · Answer 3 · answered Jan 10 '13 at 14:47

The problem is due to the way you declare your arrays:

O *o[/*No size here*/];
F *f[/*No size here*/];

Since you do not state the size of the arrays, this is equivalent to

O **o;
F **f;

Hence, you are declaring two members of types "pointer to pointer to O" and "pointer to pointer to F" respectively, but these are uninitialized and you have not allocated any memory for them to point to. That is, you actually don't have any arrays, just pointers which could be used to refer to the type of array you want.

If you know at compile time what size you want to use, you should specify that size in the declaration, which will give you a properly allocated array of that size. Otherwise, consider using an std::vector.

Pointer to object arrays as members overwrite memory

3 Answers3