0

I've got access violation error after returning Foo1 object with pointer to Foo class. Result of function Foo2::something(int) is Foo1 object . The problem is : Foo** array of Foo2::something(int) result is inaccessible.

class Foo{
  int x;
public: 
  int getX() { return x; }
 Foo& operator=(const Foo &rhs){
  x = rhs.x; 
  return *this;
}
};
class Foo1{
  Foo** array;
  int size;
public:
  Foo1(int size){
   array = new Foo*[size]
   for(int i=0; i < size;i++)
       array[i] = new Foo[size];
  }  
 Foo1(const Foo1& foo): array(foo.array), size(foo.size){}
 ~Foo1(){
    for(int i=0; i < size);i++)
       delete[] array[i];
    delete[] array;
  }
 Foo getFoo(int x, int y){
    return array[x][y];
 }
 void setFoo(int x,int y,Foo foo){
    array[x][y] = foo;
 }
 Foo1& operator=(const Foo1& foo){
   array = foo.array;
   size = foo.size;
 }
};
class Foo2{
public:
  Foo1 something(int size){
    Foo1 obj(size);
    return obj;         
  }
};
int main(){
 Foo2 foo2;
 Foo1 obj = foo2.something(3);
 obj.getFoo(0,0).getX(); // <- access violation here 
}
hwg
  • 65
  • 1
  • 9
  • 1
    What about `std::vector` instead of `Foo*`? – Zeta Dec 09 '14 at 06:50
  • 3
    Your Foo1 class's copy constructor does not make a copy of the array; it only copies the pointer. Thus after a Foo1 object is copied, both the original and the copy are now pointing to the same Foo array, and when one of the Foo1 objects gets deleted, it deletes the Foo array, leaving the other object with a dangling pointer that causes a crash (or worse) when it tries to access the array. To avoid this, your copy constructor (and also your assignment operator) need to allocate a new array and copy over the old array's contents-or better yet, follow @Zeta's advice and avoid dynamic allocation. – Jeremy Friesner Dec 09 '14 at 06:54
  • BTW, 0xFEEEFEEE means you are accessing freed heap memory: http://stackoverflow.com/a/127404/487892 – drescherjm Dec 09 '14 at 06:56
  • Thanks for quick reply I'll try allocate new array in my copy constructor and assignment operator. – hwg Dec 09 '14 at 07:00
  • Unless you have some very good reason to do otherwise, I strongly recommend to use std::vector instead of a C array. – Simon Dec 09 '14 at 08:17

1 Answers1

1
Foo1 something(int size){
  Foo1 obj(size);
  return obj;         
}

You create a object obj and return it. Lets check your copy constructor whether it takes care of copying the data:

Foo1(const Foo1& foo): array(foo.array), size(foo.size){}

That's a shallow copy: the array's contents aren't copied, instead, both the temporary object and Foo1 obj in main are going to have member array pointing to the same data. However, what happens afterwards?

Foo1 obj = foo2.something(3);
//    what happens here      ^^^^^ ?

At that point the obj in something is going to be destroyed. Therefore, its destructor is going to be called. And that very destructor deletes the contents of your array.

Either make a deep copy (allocate new memory and copy the array), use reference counting (to make sure that you don't delete the array prematurely) or use std::vector< std::vector<Foo> > or something similar which hides the plumbing.

Further notes: Enable compiler warnings. Most of your non-void functions don't use return.

Zeta
  • 103,620
  • 13
  • 194
  • 236