What is wrong with this operator overloading code?

Question

What is wrong with this c++ code?

It compiles, but, it doesn't run.

    #include <iostream>

using namespace std;

class MyClass
{
private:
    int *x;
    int *y;
public: 
    MyClass()
    {
        x = new int[1]; *x = 0;
        y = new int[1]; *y = 0;
    }
    ~MyClass()
    {
        if(x != NULL)
        {
            delete [] x;
            x = NULL;
        }
        if(y != NULL)
        { 
            delete [] y;
            y = NULL;
        }
    }
    MyClass operator=(MyClass & rhs)
    {
        MyClass temp;
        temp.Set(rhs.x[0], rhs.y[0]);
        return temp;
    }
    void Set(int a, int b)
    {
        if(x != NULL)
        {           
            *x = a;
        }
        if(y != NULL)
        {           
            *y = b;
        }
    }
    void Show()
    {
        cout<<x[0]<<y[0]<<endl;
    }
};

int main()
{
    MyClass o1;
    o1.Set(10, 20);
    o1.Show();

    MyClass o2;
    o2 = o1;
    o2.Show();
}

Answer 1

The only obvious error I can see is that you allocated x and y as arrays, so you need to use delete [] x; instead of delete x;, and the same for y. The assertion seems to also indicate that it is throwing during deletion, as the file mentioned in the assertion message is dbgdel.cpp, and it is checking whether a block type is valid (most likely, this checks whether a block to be deleted is actually an array block or a single instance)

You also don't need those checks against NULL; new always succeeds or throws an exception.

More information: delete vs delete[] operators in C++

EDIT: Also, you need to define a copy constructor. See the rule of three.

Answer 2

While the actual crash may be caused by the mismatched deallocation of the array as mentioned by rlbond's answer, there is another subtler alternative:

operator= returns a copy (not a reference as is conventional), which created using the default copy constructor. In this example it is not used and so a temporary is created. The temporaries x and y are pointing to the same memory as the copy, since the copy constructor is not defined. When the temporary is destroyed its x and y are deleted - then later o2 is destroyed resulting in the same memory getting deallocated twice.

It's worth noting that there's still a lot that can go wrong with this class, and it needs some further love and attention. Also worth seeing how we can find this error.

Here's what I get when pushing it through the pc-lint online linter. Note that this includes the problem causing the crash, and a lot of other things that need fixing.

 1  #include <iostream>
 2  
 3  using namespace std;
 4  
 5  class MyClass
 6  {
 7  private:
 8      int *x;
 9      int *y;
10  public: 
11      MyClass()
12      {
13          x = new int[1]; *x = 0;
14          y = new int[1]; *y = 0;
15      }
16      ~MyClass()
17      {
18          if(x != NULL)
19          {
20              delete x;
21              x = NULL;
22          }
23          if(y != NULL)
24          { 
25              delete y;
26              y = NULL;
27          }
28      }
29      MyClass operator=(MyClass & rhs)
        _
30      {

diy.cpp 30 Info 1722: assignment operator for class 'MyClass' does not return a reference to class

diy.cpp 30 Info 1720: assignment operator for class 'MyClass' has non-const parameter

31          MyClass temp;
32          temp.Set(rhs.x[0], rhs.y[0]);
33          return temp;
34      }
35      void Set(int a, int b)
36      {
37          if(x != NULL)
38          {           
39              *x = a;
40          }
41          if(y != NULL)
42          {           
43              *y = b;
44          }
45      }
46      void Show()
47      {
48          cout<<x[0]<<y[0]<<endl;
49      }
                _
13          x = new int[1]; *x = 0;

diy.cpp 13 Info 1733: new in constructor for class 'MyClass' which has no copy constructor

20              delete x;

diy.cpp 20 Warning 424: Inappropriate deallocation (delete) for 'new[]' data

25              delete y;

diy.cpp 25 Warning 424: Inappropriate deallocation (delete) for 'new[]' data

33          return temp;

diy.cpp 33 Info 1772: Assignment operator 'MyClass::operator=(MyClass &)' is not returning *this

34      }

diy.cpp 34 Warning 1529: Symbol 'MyClass::operator=(MyClass &)' not first checking for assignment to this

diy.cpp 34 Info 1764: Reference parameter 'rhs' (line 29) could be declared const ref

diy.cpp 34 Warning 1539: member 'MyClass::x' (line 8) not assigned by assignment operator

diy.cpp 34 Warning 1539: member 'MyClass::y' (line 9) not assigned by assignment operator

48          cout<<x[0]<<y[0]<<endl;

diy.cpp 48 Warning 613: Possible use of null pointer 'MyClass::x' in left argument to operator '[' [Reference: file diy.cpp: line 37]

diy.cpp 48 Warning 613: Possible use of null pointer 'MyClass::y' in left argument to operator '[' [Reference: file diy.cpp: line 41]

49      }

diy.cpp 49 Info 1762: Member function 'MyClass::Show(void)' could be made const

50  };
51  
52  int main()
53  {
54      MyClass o1;
55      o1.Set(10, 20);
56      o1.Show();
57  
58      MyClass o2;
59      o2 = o1;
60      o2.Show();
61  }
62