operator= error

DrZoo (8)
My program is crashing when I'm trying to call b = a. What has me confused is that when I call c = d = b, it all works fine. Here is the code. I can't think of what's going wrong. Maybe I just need a break...

EDIT: I actually think I see it now that I posted this. In operator= I'm changing the size, but I'm not changing the size of the array that it's pointing to correct? I'm keeping it as 'p = new T[size]' rather than changing it to 'p = new T[x.size]' correct?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
 
#include <iostream>
#include "Array.h"


int main()
{
    std::cout << "creating Array< int > object a ...\n";
    Array< int > a(5);
    a[0] = 42; // a.operator[](0) = 42
    a[1] = 0;
    a[2] = 0;
    a[3] = 0;
    a[4] = -5000;
    
    Array< int > b(3);
    b = a; // b.operator=(a)  ***** problem occurs here*****
    std::cout << b << std::endl;
    Array< int > c(0), d(0);
    c = d = b;
    std::cout << c << '\n';
    std::cout << d << '\n';
    Array< int > e(3);
    for (int i = 0; i < 3; ++i)
    {
        e[i] = i;
    }
    std::cout << "e : " << e << std::endl;


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
 
#ifndef ARRAY_H
#define ARRAY_H

#include <iostream>


class IndexError
{};


template < class T >
class Array
{
public:
    Array(int size0)
        : size(size0), p(new T[size])
    {};
    
    // TODO: Set attributes, copy values
    // at x.p to p
    Array(const Array & x)
        : size(x.size), p(new T[x.size]) //x.size
    {
        for (int i = 0; i < x.size; ++i)
        {
            p[i] = (x.p)[i];
        }
    };
    
    Array(T arr[], int n) 
    {
        // non-class array into the class
    };
    
    ~Array()
    {
        //std::cout << "~Array()" << std::endl;
        delete [] p;
    };
    
    int get_size() const
    {
        return size;
    }
    
    T operator[](int index) const
    {
        if (index < 0 || index >= size)
        {
            throw IndexError();
        }
        // FIXIT
        return p[index];
    }
    
    T & operator[](int index)
    {
        if (index < 0 || index >= size)
        {
            throw IndexError();
        }
        return p[index];
    }
    
    Array & operator=(const Array & x)//********* this operator ***************
    {
        //std::cout << "size: " << size << std::endl;
        //std::cout << "x.size: " << x.size << std::endl;
        if (this != &x)
        {
            size = x.size;
            for (int i = 0; i < x.size; ++i)
            {
                p[i] = (x.p)[i];
            }
        }
        return (*this);
    }
    
    bool operator==(const Array & x) const
    {
        if (size != x.size)
        {
            return false;
        }
        for (int i = 0; i < size; ++i)
        {
            if (p[i] != (x.p)[i])
            {
                return false;
            }
        }
        return true;
    }

    bool operator!=(const Array & x) const
    {
        return !(*this == x);
    }
    
    void insert(int index, const T & value)
    {
        if (index < 0 || index >= size)
        {
            throw IndexError();
        }
        else
        {
            p[index] = value;
        }
    }
private:
    int size;
    T * p;
};



template < class T >
std::ostream & operator<<(std::ostream & cout,
                          const Array< T > & x)
{
    int size = x.get_size();
    cout << '[';
              for (int i = 0; i < size - 1; ++i)
              {
                  cout << x[i] << ", "; // Recall: x[i] calls x.operator[](i)
              }
              cout << x[size - 1];
              cout << ']';
    return cout;
}

#endif
Last edited on
KRAkatau (197)
Well, the problem is that you allocate only three elements for array b and you have five in array a. Therefore you write out of bounds. What you need to do here is: delete the old p for array you copy to and allocate a new array of the fitting size before copying.

That's pretty much it. Cheers.
KRAkatau (197)
Your edit is correct. Don't forget to free the memory pointed to with "p" before allocating new mem.
DrZoo (8)
That was another question of mine. I have the destructor

~Array() { delete [] p}

but that's called upon itself, right? I don't need to make a call before allocating? If I remember correctly you NEVER call the destructor. It's always called for you. You just may have to make modifications on what it does, based on your code?
Last edited on
vlad from moscow (6539)
It is obvious that your copy assignment operator has some problems.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
    Array & operator=(const Array & x)//********* this operator ***************
    {
        //std::cout << "size: " << size << std::endl;
        //std::cout << "x.size: " << x.size << std::endl;
        if (this != &x)
        {
            size = x.size;
            for (int i = 0; i < x.size; ++i)
            {
                p[i] = (x.p)[i];
            }
        }
        return (*this);
    }


the left and the right operands can have different number of elements. in this case after statement

size = x.size;

the value of size can contradict the actual size of the array. I think that you should at first delete the current array and allocate a new array with number of elements equal to x.size
Last edited on
DrZoo (8)
Yeah vlad, I changed it to this.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
 Array & operator=(const Array & x)
    {
        //std::cout << "size: " << size << std::endl;
        //std::cout << "x.size: " << x.size << std::endl;
        if (this != &x)
        {
            p = new T[x.size];
            size = x.size;
            for (int i = 0; i < x.size; ++i)
            {
                p[i] = (x.p)[i];
            }
        }
        return (*this);
    }


I'm just wanting to make sure the deallocation of memory has been handled correctly by

1
2
3
4
5
 
~Array()
    {
        //std::cout << "~Array()" << std::endl;
        delete [] p;
    };
Last edited on
S G H (2638)
Nah.
~Array isn't called at all.
You should delete[] p before allocating a new buffer.
vlad from moscow (6539)
Before allocating a new array you have to delete the old one in your copy assignment operator.

I would write it the following way

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
 Array & operator=(const Array & x)
    {
        //std::cout << "size: " << size << std::endl;
        //std::cout << "x.size: " << x.size << std::endl;
        if (this != &x)
        {
            T *tmp = new T[x.size];
            for (int i = 0; i < x.size; ++i)
            {
                tmp[i] = (x.p)[i];
            }
            delete []p;
            size = x.size;
            p = tmp;
        }
        return (*this);
    }
Last edited on
DrZoo (8)
Ok, so transfer the passed in array into some temp array, then delete [] p, then allocate the new array?
Cubbi (4774)
You could always take the lazy way out and let your copy-constructor and the destructor do all the work

1
2
3
4
5
6
 
Array& operator=(Array x) // copy-constructor called here
{
    std::swap(size, x.size);
    std::swap(p, x.p);
    return *this;
} // destructor called here


although in this particular case, it is suboptimal when assigning arrays of the same size.
vlad from moscow (6539)
@Cubbi


I do not think that it is a lazy way because the copy constructor is called for the parameter of the copy assignment operator.
Cubbi (4774)
by "lazy" I mean that it involves the least amount of typing
vlad from moscow (6539)
@Cubbi


Ah, I see.
DrZoo (8)
Thanks everyone for your input and help. I greatly appreciate it!
Topic archived. No new replies allowed.