Stack corruption by class destructor

I've defined a matrix class in C++, with a single data_[] array, and an overloaded () operator to access it. It worked fine as a container, but I defined an addition operation:

Code:

---

matrix matrix::operator +(const matrix op1)
{
        if((op1.rows_==rows_)&&(op1.cols_==cols_))
        {
                matrix result(rows_,cols_);
                for(int r=0;r<rows_;r++)
                {
                        for(int c=0;c<cols_;c++)
                        {
                                result(r,c)=op1(r,c)+data_[(cols_*r)+c]; // fill result matrix with sums
                        };
                };
                return result;
        }else{
                cout << "matricies are not of equal dimension" << endl;
                matrix ret(1,1);
                return ret;
        };
};

---

Now, I added two matrices of equal dimension, and it went into the first branch of the conditional. It went fine, I followed it step by step, it filled result with the sums. It worked fine when they weren't equal as well. But once it's passed "return result;", and exits the function, after destroying the internal variables, it steps back to the addition expression, and calls the destructor:

Code:

---

matrix::~matrix()
{
        delete[] data_;
};

---

And the delete[] causes an access violation. I can't figure out what it's trying to delete. Any ideas?

It is deleting the temporary object that the compiler uses for the return value.

--
Mats

Right, that makes sense. But then, why does delete [] cause a violation? It works fine when matrices are deleted at the end of main{}. I can't see why it would cause a violation just because it's deleting a temp. Do I need a conditional in the destructor?

It obviously shouldn't cause a problem - but there's probably something else wrong in your class, such that when it deletes the temporary it upsets something. Try putting a tracer in the constructor and destructor for _data[]. They should always be matched pairs.


--
Mats

The problem might be invalid copying. Did you define a copy constructor and copy assignment operator? If not, then you must do that because you are using plain dynamic arrays. They don't get copied correctly by default.

In addition, I have yet to hear any reason to use a plain dynamic array rather than a vector in C++ in situations like this. I would switch to vector rather than the dynamic data array. If the lack of proper copying is your problem, then you would have avoided the issue by using vector. You also wouldn't even need to write a destructor, since the vector would clean itself up automatically. Unless you need to use dynamic arrays for learning purposes I'd encourage you to switch to the C++ vector container.

I do not have a copy constructor. The reason that I'm not using vectors is because (a) I don't know what they are, (b) I've been told this is the most efficient solution.

Right, off to write a copy constructor then. Thanks!

Don't forget the assignment operator.

Quote:

---

(a) I don't know what they are

---

A vector is just that - a (smart) dynamically allocated array. Except it can look after its own memory and therefore doesn't require you to write neither a destructor, copy constructor or an assignment operator for your class in this case.

Quote:

---

(b) I've been told this is the most efficient solution.

---

Since the vector is very little different that a simple dynamically allocated array, the performance shouldn't really be any different. (If it comes to arrays with unknown final size, it might most probably be more efficient that what you would come up with.)

A vector is a class in the C++ standard library. It is just as efficient as a dynamic array. In fact, it is really just a dynamic array wrapped in a nice package to help prevent bugs like this.

If you're planning on doing anything beyond beginner C++, you really should learn it. (I hope you're using the C++ string class for your strings as well.) Unfortunately many books/tutorials/instructors sometimes teach old and out of date practices when there are better solutions available. This is one of those examples.

Still, it can't hurt to practice your copy constructor. Don't forget the copy assignment operator, too, the rule of three states that if you need a destructor, copy constructor or copy assignment operator, you probably need all three.

Yay! It works!

Okay, vectors sound important. I'll learn them, seeing as I'm making matrices with a view to implementing AES (I like to jump in head first, and explore).

I actually had a copy assignment operator before, I don't know why I didn't think to make a copy constructor. Oh well, I'm just human I guess.

And yeah, <string> is very useful, you'd have to be insane not to use it.

Well, I've gotten through this, and I'm the wiser for it. Thanks guys!

Hi, me again. Is there any way to change this code:

Code:

---

ostream& operator << (ostream& os, const matrix& m)
{
        int r,c;
        ostream out;
        for(r=0; r<m.rows_; r++)
        {
                for(c=0; c<m.cols_; c++)
                {
                        out=out << m(r,c);
                        out=out << " ";
                };
                out=out<<endl;
        };
        return os << out;
};

---

so that it will work? It's telling me that it can't find a constructor for 'ostream'.

>> ostream out;
Remove this line. You don't need to declare an ostream, just write everything to os.

Then, use os exactly as you would use cout. You wouldn't do this:

Code:

---

cout = cout << m(r, c);

---

Would you? So don't do it with os.

Finally, when you are done, just return os.

Thanks :)

I've even got it working with vectors now. I've been able to cut out a few conditionals due to the exception throwing. Now, maybe I should try making an "invalid matrix operation" exception for the rest...