I've got some problems with a c++ program I'm working on. . .I believe the problem is the copy constructor, which I don't know how to write properly, I guess. Any help is appreciated.
Files here
Do not try to bend the spoon, that's impossible. Instead only try to realize the truth.
What Truth?
There is nospoon. Then you will see it is not the spoon that changes, it is only yourself
Your code has:
typedef double* doubleArrayPtr;
doubleArrayPtr* array;
In other words, you're defining array as double**, that is, a pointer to a pointer to a double. Is this really what you want?
Hmm. I guess it is, because you handle it properly.
Your code is missing the assignment operator: matrix& operator= (const matrix& ref);
It should be implemented in the same way as the copy constructor, which by the way seems to be allright (except for a missing semicolon before "for").
You would have noticed this if you had used the -Weffc++warning option. I recommend you add -Wall -W -Weffc++ to your Makefile in the CXXFLAGS.
Why did you put using namespace std; into matrix.h? There's nothing in matrix.h that uses anything from namespace std. You can use it in matrix.cpp if you like, but don't put it into the header file for it corrupts the namespace of everyone who uses that header.
Yeah, I use a pointer to a pointer to a double to make a 2d matrix.
I'll fix that other stuff after class.
THANKS!!!!
Do not try to bend the spoon, that's impossible. Instead only try to realize the truth.
What Truth?
There is nospoon. Then you will see it is not the spoon that changes, it is only yourself
Okay, now it works. I added that line you suggested to the makefile and I get a bunch of warnings. It still runs fine, but how do I fix those warnings(or do I)? Files are still here
Do not try to bend the spoon, that's impossible. Instead only try to realize the truth.
What Truth?
There is nospoon. Then you will see it is not the spoon that changes, it is only yourself
By following what they say.
> warning: 'operator=' should return a reference to '*this'
Solution: Change your operator= so that it returns a reference to *this.
Instead of void operator=(const matrix& rhs) you should use: matrix& operator= (const matrix& rhs).
In case you are wondering what that function should return, it's return *this;.
> matrix.cpp:4: warning: 'matrix::array' should be initialized in the member initialization list
> matrix.cpp:4: warning: 'matrix::m' should be initialized in the member initialization list
> matrix.cpp:4: warning: 'matrix::n' should be initialized in the member initialization list
Solution: Instead of
matrix::matrix()
{
m = 0;
n = 0;
}
you should write:
matrix::matrix()
: array(0), m(0), n(0)
{
}
And so on.
The warnings help avert common design / implementation mistakes, so one should heed them.
In this case, the array(0) prevents your program from crashing if you do void foo() { matrix a; }. With your previous code, it would leave the "array" property uninitialized (i.e. its value would be random), and when the destructor runs, it would attempt to delete the memory pointed by that undefined pointer. It is however safe to attempt to delete with a null pointer, which is why you should set it to 0 if you haven't allocated anything.
Ps: Do not forget to deallocate the previous contents of the matrix in the assignment operator! Otherwise, your code has a resource leak.
PPs: Do not forget that "a = a;" is legal code. The "rhs" of the assignment operator might actually be the object itself. Verify that your program does not do anything unexpected if you do that.
Coding self-made array classes in C++ is rather tricky to get 100% correct. Dynamic allocation of memory is precisely one of the things which are most diffiult in C++ and it's very easy to shoot oneself in the foot.
Unless you have very good reasons to not to use std::vector ("I think it's inefficient" is not a good reason because you are probably wrong; "I don't know how it works" is not a good reason either), I recommend using that instead of allocating arrays "by hand".
In general, you should avoid 'new' as far as possible. If you can avoid 'new' by eg. using STL containers, you should always prefer doing so unless you have a good reason why none of the STL containers is efficient enough for what you are doing.
Sometimes having to use 'new' is unavoidable. In that case you should always remember good encapsulation, and you should really learn how constructors, destructors, copy constructors and assignment operators should be created for such classes.
The easiest way of avoiding problems with classes which allocate memory is to simply disallow the copy constructor and the assignment operator. This is done by declaring them in the private part of the class (and not implementing them). Often these types of containers can be used without the need for copying or assignment.
If you simply can't avoid having to copy and/or assign these objects, then you'll have to be very meticulous about the copy constructors and assignment operators. I recommend the books "Efficient C++" and "More Efficient C++". They teach these and many other useful things to know.
