-2

I hava a class

class vlarray {
public:
    double *p;
    int size;

    vlarray(int n) {
        p = new double[n];
        size = n;
        for(int i = 0; i < n; i++)
            p[i] = 0.01*i;
    }

    ~vlarray() {
        cout << "destruction" << endl;
        delete [] p;
        size = 0;
    }
};

when I use in main

int main() {
    vlarray a(3);
    {
        vlarray b(3);
        b.p[0] = 10;
        for(int i = 0; i < 3; i++) {
            cout << *(b.p+i) << endl;
        }
        a = b;
    }    // the magic happens here deallocation of b
    for(int i = 0; i < 3; i++) {
        cout << *(a.p+i) << endl;
    return 0;
}

when b deallocated smth happens to a .. what is the problem, why that problem occurs and how to avoid such type of problems?

ildjarn
  • 62,044
  • 9
  • 127
  • 211
nurmurat
  • 151
  • 1
  • 11

7 Answers7

5

There seems to be some confusion amongst the existing answers on this question, so I am going to jump in.


Immediate issue

Your primary issue is that you have not defined your own copy assignment operator. Instead, the compiler generates a naive one for you, so that when you run a = b, the pointer inside b is copied into a. Then, when b dies and its destructor runs, the pointer now inside a is no longer valid. In addition, a's original pointer has been leaked.

Your own copy assignment operator will need to delete the existing array, allocate a new one and copy over the contents from the object you're copying..

More generally

Going further, you will also need to define a few other things. This requirement is neatly summed-up as the Rule of Three (C++03) or Rule of Five (C++11), and there are plenty of explanations online and in your favourite, peer-recommended C++ book that will teach you how to go about satisfying it.

Or, instead...

Better still, you could start using an std::vector instead of manually allocating everything, and avoid this entire mess:

struct vlarray {
    std::vector<double> p;

    vlarray(int n) {
        p.resize(n);
        for(int i = 0; i < n; i++)
            p[i] = 0.01*i;
    }
};
Community
  • 1
  • 1
Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
4

You need to follow the Rule of Three in C++03 and Rule of Five in C++11.

Background and Basis of these Rules:

Whenever your class has an pointer member with an dynamic memory allocation, and whenever another object is created from this existing object by using any of the Copying Functions(Copy constructor & Copy Assignment Operator in c++03) unless you overload these two make a Deep Copy of the member pointer, the newly created object will keep pointing to the memory allocation of the parent object(Shallow Copy). The problem occurs when the parent object gets destroyed( for ex: by going out of scope) it's destructor gets called, which usually would free the memory allocated to the pointer member, When this happens the objects with a shallow copy of this pointer now point to invalid memory region and become Dangling Pointers. These dangling pointers when accessed result in Undefined Behavior and most likely crashes.

To avoid this you need to follow the Rule of Three in C++03 and Rule of Five in C++11.
The difference of Rules in C++03 and C++11 because the functions that control the copying behavior for a class have changed in C++11.

Rule of Three Basically states:
Implement the copy constructor, copy assignment operator and Destructor for your class.

EDIT:
If you are using C++11, then the Rule of Three actually becomes Rule of Five.

Community
  • 1
  • 1
Alok Save
  • 202,538
  • 53
  • 430
  • 533
  • This post is tagged `c++11`, which means that [the "Rule of Five" is more appropriate](http://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c0x). – Lightness Races in Orbit Sep 21 '11 at 17:55
  • @TomalakGeret'kal: Ah right, I missed the C++11 tag, I edited to reflect that, Thanks for noticing that detail. – Alok Save Sep 21 '11 at 17:58
0

What you didn't implement is called:

which in short says,

The rule of three (also known as the Law of The Big Three or The Big Three) is a rule of thumb in C++ that claims that if a class defines one of the following it should probably explicitly define all three:

  1. destructor
  2. copy constructor
  3. copy assignment operator

Since you tagged your question C++11 as well, then you have to implement this:

Community
  • 1
  • 1
Nawaz
  • 353,942
  • 115
  • 666
  • 851
0

I think the problem is that you don't have a copy constructor. The compiler tried to do its best but it doesn't always succeed. Write a copy constructor and try again.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • Actually the issue is a missing user-defined assignment operator (though a copy constructor would be wise too). And the compiler doesn't "try to do" or "fail" at anything: what is happening is completely well-defined up until the OP decides to `delete` the same block of memory twice. – Lightness Races in Orbit Sep 21 '11 at 17:53
  • -_- copy constructor would work ... don't have to -1 me. You are right though an operator is what should be defined. – Padawan Learner Sep 21 '11 at 17:56
  • No, a copy constructor does not fix this problem, as `a=b` invokes the assignment operator, not the copy constructor. That's why you got a -1. – Lightness Races in Orbit Sep 21 '11 at 17:58
0

The destruction of b caused the member pointer p to be deleted. Since this pointer was copied to a, the member pointer of a is now pointing to deleted memory. You have undefined behavior.

To avoid this you need to do a deep copy at any point where you copy one object to another, typically the copy constructor and assignment operators. Allocate a new array and copy all the elements from one array to the other. You now adhere to the Rule Of Three because you've already defined a destructor.

The best way around this problem is to avoid raw pointers altogether and replace them with std::vector.

Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
-1

Direct asignment shares P between instances. Create a copy constructor.

robermorales
  • 3,293
  • 2
  • 27
  • 36
-1

When you assign b to a (a=b) the the compiler gerneated copy constructor does a shallow copy on your data members. I does not deference the pointers. So they both share the same resource, and when either of them are destroyed then they take their shared resource with them

Either define your own copy constructor to do a deep copy, or use an array abstraction like vector.

class vlarray {
public:
  std::vector<double> p;
  int size;
  vlarray(int n) {
    p.resize(n);
    size = n;
    for(int i = 0; i < n; i++) p[i] = 0.01*i;
  }
  ~vlarray() {
    cout << "destruction" << endl;
    }
};
Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
111111
  • 15,686
  • 6
  • 47
  • 62