Discussion:
Why does this snippet crash?
(too old to reply)
KK
2004-08-09 23:44:26 UTC
Permalink
Hello all,
template <class matType=int> class matrix {
public:
int rows;
int cols;
matType **element;
matrix(){};
matrix(int rows, int cols, matType fillValue=0);
matrix(const matrix &copy);
~matrix();
matrix operator=(matrix A);
};

template<class matType>
matrix<matType>::matrix (int rows, int cols, matType fillValue){
register int i,k;
this->rows=rows;
this->cols=cols;
element= new matType* [rows];
for(i=0;i<rows;i++){
element[i]= new matType [cols];
for(k=0;k<cols;k++){
element[i][k]=fillValue;
}
}
}
template<class matType>
matrix<matType>::~matrix(){
for(int i=0;i<rows;i++)
delete [] element[i];
delete [] element;
}
template<class matType>
matrix<matType>::matrix(const matrix &copy){
int i,k;
rows=copy.rows;
cols=copy.cols;
element= new matType* [copy.rows];
for(i=0;i<copy.rows;i++){
element[i]= new matType [copy.cols];
for(k=0;k<copy.cols;k++)
{
element[i][k]=copy.element[i][k];
}
}
}
matrix<matType> matrix<matType>::operator=(matrix<matType> A)
{
int i,k;
rows=A.rows;
cols=A.cols;
for(i=0;i<rows;i++){
for(k=0;k<cols;k++){
element[i][k]=A.element[i][k];
}
}
return *this;
}
template <class matType>
matrix<matType> Transpose(matrix<matType> A)
{
matrix<matType> B(A.cols,A.rows);
for(register int i=-1;++i<A.rows;)
for(register int k=-1;++k<A.cols;)
B(k,i)=A(i,k);
return B;
}
void main(){
matrix<double> D(3,4,1);
D=Transpose(D);
}

Any suggestions/Corrections in the code is highly appreciated.
Best regards,
KK

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Victor Bazarov
2004-08-10 18:48:22 UTC
Permalink
Post by KK
Hello all,
[...]
Any suggestions/Corrections in the code is highly appreciated.
There are numerous errors in the program. First of all, operator= is
not implemented correctly. If matrices have different dimensions, there
is no way to simply assign elements, you need to reallocate storage for
the one on the left. Second, function signatures are out of order: many
should accept const references instead of values. Also, the return value
type of the assignment should be a reference. Third, operator()(int,int)
hasn't been implemented... And last, but not least, your 'main' has the
return type 'void'.

Care to take another pass at it?

V

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Balog Pal
2004-08-10 19:14:51 UTC
Permalink
"KK" <***@yahoo.com> wrote in message news:***@posting.google.com...

The whole thing is very, very bad, you should read at least Meyers'
Effective C++, you'll find a plenty of items applying to your code.

The most immediate couse of the crash is that your operator= is completely
broken.
Fixing it may get your program run without crash for a while, but the other
problems would soon go after you.

Paul



[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Werner Salomon
2004-08-10 19:15:41 UTC
Permalink
Post by KK
[...]
Any suggestions/Corrections in the code is highly appreciated.
Hi KK,
the reviewed code snippet:

template <class matType=int> class matrix {
public:
int rows; // make the members private
int cols;
matType **element;
matrix(){};
matrix(int rows, int cols, matType fillValue=0);
matrix(const matrix &copy);
~matrix();
// matrix operator=(matrix A);
matrix operator=( const matrix& A );

// missing operator()() [const]
matType& operator()( int row, int col )
{
return element[row][col];
}
matType operator()( int row, int col ) const
{
return element[row][col];
}
};

template<class matType>
matrix<matType>::matrix (int rows, int cols, matType fillValue)
// use initialization list
: rows( rows ) // mark Your members with '_' or 'm_'
, cols( cols )
, element( 0 )
{
register int i,k;
this->rows=rows; // s. above
this->cols=cols;
element= new matType* [rows];
for(i=0;i<rows;i++){
element[i]= new matType [cols];
for(k=0;k<cols;k++){
element[i][k]=fillValue;
}
}
}
template<class matType>
matrix<matType>::~matrix(){
for(int i=0;i<rows;i++)
delete [] element[i];
delete [] element;
}
template<class matType>
matrix<matType>::matrix(const matrix &copy){
// use initialization list (s.above)
int i,k;
rows=copy.rows;
cols=copy.cols;
element= new matType* [copy.rows];
for(i=0;i<copy.rows;i++){
element[i]= new matType [copy.cols];
for(k=0;k<copy.cols;k++)
{
element[i][k]=copy.element[i][k];
}
}
}

template<class matType> // <- this line was missing
matrix<matType> matrix<matType>::operator=(const matrix<matType>& A)
// matrix<matType> matrix<matType>::operator=(matrix<matType> A)
{
// check for self assign -> if( this != &A ) ...
int i,k;
rows=A.rows;
cols=A.cols;
// here You have to free and reallocate the memory !
// crashes if rows < A.rows || cols < A.cols
for(i=0;i<rows;i++){
for(k=0;k<cols;k++){
element[i][k]=A.element[i][k];
}
} return *this;
}
template <class matType>
//matrix<matType> Transpose(matrix<matType> A)
matrix<matType> Transpose( const matrix<matType>& A)
{
matrix<matType> B(A.cols,A.rows);
for(register int i=-1;++i<A.rows;)
// why not: for( int i=0; i<A.rows; ++i )
for(register int k=-1;++k<A.cols;)
B(k,i)=A(i,k);
return B;
}

int main(){
matrix<double> D(3,4,1);
D=Transpose(D);
return 0;
}

Your code crashes in the operator= because You assign a 4x3-matrix to
a 3x4-matrix.
Think about storing the matrix elements in one (!) array and using a
STL-container (std::vector< matType >) instead the array; this would
halves Your code and double the correctness.

Greetings
Werner

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Michaelangelo
2004-08-11 01:21:43 UTC
Permalink
On 9 Aug 2004 19:44:26 -0400, ***@yahoo.com (KK) wrote:

This looks like a homework type question. Regardless...
since you could use some tips...

1. It is customary to prefix member variables with either m_, _, or
suffix them with an underscore.

2. The default ctor is ill-formed. It doesn't initialize the size, or
the pointer of elements.

3. There is a typo in return type of the the assignment operator.
The declaration returns a temporary, yet you are returning a reference
in the definition.

4. "register" is archaic. All modern C++ compilers make efficient use
of all CPU registers.

5. Transpose tries to assign elements, but does so without
either referring to elements, or using a overloaded operator() to do
it.

6. rows, and cols, are accessible publically, yet the size of the 2d
matrix depends on them. They should be made with getters/setters, so
that the element array can be re-dimensioned if they change.

7. Tradionally, lhs, and rhs, are used for the names of functions that
deal with unary, and binary operations (such as assignment or copy) on
the class.

8. Before writing template code, it is best to write (& debug!) it as
non-templatized. Once it is working, THEN convert it to a template.
This is because if you start off with templatized code, it still may
have compile errors if you never reference it. The non-templatized
version will have the compiler flag ALL errors.

9. Extra whitespace never hurts!!

10. When you see common operations throughout your codebase, create
functions for them. The point of programming is to create, manage,
and use BOTH the micro, and macro.

11. Did you intentionally want Transpose NOT to be a member function?
Since it deals with the internals it should be a member function (or a
friend). Furthermore, this type of operation is natural to have 2
ways of being called, since that would be helpfull to users of your
class.

12. Learn to read (& understand) other people's code -- you'll become
a better writer in the process. Everyone has their own coding style,
but I recommend you seperate your functions via comments, at the very
least.

13. I would recommend a few C++ books.
- The C++ Programming Language (Third Edition), by Stroustrup
- Thinking in C++, by Bruce Eckel
- All books by Scott Meyers, and Andrei Alexandrescu
- ARM (although quite old, it is still very usefull)


Here is the code with all the changes...

As a bonus, I've included the ability to compile it as as template, or
non-template code, so you can see how to minimize your conversions in
the future.


Cheers



// Matrix N x M _____________________________________________________

#include <iostream>

using std::cout;

// Uncomment for template version.
// Comment out for non-template version.
#define TEMPLATE

#ifdef TEMPLATE
#define TEMP_DEF template <typename TYPE = int>
#define TEMP template <typename TYPE>
#define SELF matrix<TYPE>
#else
#define TEMP_DEF
#define TEMP
#define SELF matrix
// Template Parameter/Argument name must match TEMP_DEF and
TEMP
#define TYPE double
#endif

TEMP_DEF
class matrix
{
int _nRows;
int _nCols;
TYPE **_aElement;

typedef TYPE by_value ;
typedef TYPE& by_ref ;

void _Create(const int & nRows, const int & nCols, const TYPE&
nFill )
{
_nRows = nRows;
_nCols = nCols;
_aElement = new TYPE* [nRows];
for( int y = 0; y < nRows; y++ )
{
_aElement[y] = new TYPE [_nCols];
for (int x = 0; x < nCols; x++ )
{
_aElement[y][x] = nFill;
}
}
}


// Elements are NOT initialized
void _Create(const int & nRows, const int & nCols )
{
_nRows = nRows;
_nCols = nCols;
_aElement = new TYPE* [nRows];
for( int y = 0; y < nRows; y++ )
{
_aElement[y] = new TYPE [_nCols];
}
}

void _Destroy()
{
for( int y = 0; y < _nRows; y++)
delete [] _aElement[y];
delete [] _aElement;
_aElement = NULL;
}

// assumes "this" is the same size as "rhs"
void _Copy(const matrix & rhs)
{
for( int y = 0; y < _nRows; y++ )
{
for (int x = 0; x < _nCols; x++ )
{
_aElement[y][x] = rhs._aElement[y][x];
}
}
}

public:
// ctor
matrix()
: _nRows( 0 )
, _nCols( 0 )
, _aElement( 0 )
{
}

matrix(const int& _nRows, const int& _nCols, const TYPE&
fillValue=0);

// copy-ctor
matrix(const matrix &rhs);

//dtor
~matrix();

// assignment
SELF& operator=(const SELF & rhs);

void Set( const int& x, const int & y, const TYPE& nNewElement
)
{
_aElement[y][x] = nNewElement;
}

TYPE& Get( const int& x, const int & y) const
{
return _aElement[y][x];
}

int GetRows() const
{
return _nRows;
}

int GetCols() const
{
return _nCols;
}

// Util - re-dimension
void SetRows( const int& nNewRows )
{
_Destroy();
_nRows = nNewRows;
_Create( _nRows, _nCols );
}

void SetCols( const int& nNewCols )
{
_Destroy();
_nCols = nNewCols;
_Create( _nRows, _nCols );
}

// Functionality
SELF& Transpose();
SELF& Transpose( const SELF & rhs );

// I/O
// TODO: overloading the output stream operator << is left as
an excersize for the reader.
void Print() const
{
for( int y = 0; y < _nRows; y++ )
{
for( int x = 0; x < (_nCols-1); x++ )
{
cout << _aElement[y][x] << ", ";
}
cout << _aElement[y][x] << "\n";
}
}
};


// ==================================================================
TEMP
SELF::matrix (const int& nRows, const int& nCols, const TYPE& nFill )
{
_Create( nRows, nCols, nFill );
}


// ==================================================================
TEMP
SELF::~matrix()
{
_Destroy();
}


// ==================================================================
TEMP
SELF::matrix(const SELF &rhs)
{
_Create( rhs._nRows, rhs._nCols );
_Copy( rhs );
}

// ==================================================================
TEMP
SELF& SELF::operator=(const SELF& rhs)
{
// are we not the same size as the rhs?
if ((_nRows != rhs._nRows) || (_nCols != rhs._nCols))
{
_Destroy();
_Create( rhs._nRows, rhs._nCols );
}

_Copy( rhs );
return *this;
}

// ==================================================================
TEMP
SELF& SELF::Transpose(const SELF & rhs)
{
_Destroy();
_Create( rhs._nCols, rhs._nRows ); // src ROWS & COLS
intentonally swapped

for( int y= -1; ++y < rhs._nRows; /* */ )
for( int x = -1; ++ x < rhs._nCols; /* */ )
_aElement[x][y] = rhs._aElement[y][x]; // dst
ROWS & COLS intentonally swapped
return *this;
}


TEMP
SELF& SELF::Transpose()
{
SELF orginal( *this );

_Destroy();
_Create( orginal._nCols, orginal._nRows ); // src ROWS & COLS
intentonally swapped

for( int y=-1; ++y < orginal._nRows; /* */ )
for( int x=-1; ++x < orginal._nCols; /* */ )
_aElement[x][y] = orginal._aElement[y][x]; //
dst ROWS & COLS intentonally swapped

return *this;
}

#undef TEMP_DEF
#undef TEMP
#undef SELF
#undef TYPE

// Test _____________________________________________________________

// ==================================================================
int main( int argc, char* argv[] )
{
#ifdef TEMPLATE
matrix<double> D(3,4,1);
#else
matrix D(3,4,1);
#endif

int i = 1;
for( int y = 0; y < D.GetRows(); y++ )
for( int x = 0; x < D.GetCols(); x++)
D.Set( x, y, (double) i++ );

#ifdef TEMPLATE
matrix<double> B( D );
matrix<double> T;
#else
matrix B( D );
matrix T;
#endif

cout << "Before Transpose\n";
D.Print();
T.Transpose( D );
cout << "\nAfter Transpose (copy)\n";
T.Print();

cout << "\nBefore Transpose (copy)\n";
B.Print();

cout << "\nAfter Transpose\n";
D.Transpose();
T.Print();

return 0;
}

// EOF

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Francis Glassborow
2004-08-11 19:44:51 UTC
Permalink
Post by Michaelangelo
This looks like a homework type question. Regardless...
since you could use some tips...
1. It is customary to prefix member variables with either m_, _, or
suffix them with an underscore.
Not where I come from. The suffix is acceptable but prefixing is much
frowned upon.
Post by Michaelangelo
8. Before writing template code, it is best to write (& debug!) it as
non-templatized. Once it is working, THEN convert it to a template.
This is because if you start off with templatized code, it still may
have compile errors if you never reference it. The non-templatized
version will have the compiler flag ALL errors.
I go further and suggest that the code be written and debugged using a
typedef for what will become the template parameter. This typedef should
then be used to check the code for at least three cases (usually, int,
double and at least one udt).
Post by Michaelangelo
9. Extra whitespace never hurts!!
Whitespace that causes wrapping, or expands code so it no longer fits on
the screen does hurt.
Post by Michaelangelo
12. Learn to read (& understand) other people's code -- you'll become
a better writer in the process. Everyone has their own coding style,
but I recommend you seperate your functions via comments, at the very
least.
There is a book on the subject:
Code Reading: The Open Source Perspective by Diomidis Spinellis
Post by Michaelangelo
13. I would recommend a few C++ books.
- The C++ Programming Language (Third Edition), by Stroustrup
- Thinking in C++, by Bruce Eckel
- All books by Scott Meyers, and Andrei Alexandrescu
Andrei's work is a little advanced but those from Herb Sutter should be
in this list
Post by Michaelangelo
- ARM (although quite old, it is still very usefull)
I think that The Design & Evolution of C++ would be a better choice, the
ARM has too many points of difference with Standard C++ (not surprising
as it was published circa 1989)
--
Francis Glassborow ACCU
Author of 'You Can Do It!' see http://www.spellen.org/youcandoit
For project ideas and contributions: http://www.spellen.org/youcandoit/projects


[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Michaelangelo
2004-08-12 12:07:06 UTC
Permalink
On 11 Aug 2004 15:44:51 -0400, Francis Glassborow
Post by Francis Glassborow
Post by Michaelangelo
1. It is customary to prefix member variables with either m_, _, or
suffix them with an underscore.
Not where I come from. The suffix is acceptable but prefixing is much
frowned upon.
It varies from environment to environment. That's why I provided /
listed both means. ;-)

I don't know which style is more popular; frankly it doesn't matter,
just as long as you adopt whatever coding style the project is using,
and that you be consistent.

Any styles you recommend in particular?
Post by Francis Glassborow
Post by Michaelangelo
8. Before writing template code, it is best to write (& debug!) it as
non-templatized. Once it is working, THEN convert it to a template.
This is because if you start off with templatized code, it still may
have compile errors if you never reference it. The non-templatized
version will have the compiler flag ALL errors.
I go further and suggest that the code be written and debugged using a
typedef for what will become the template parameter. This typedef should
then be used to check the code for at least three cases (usually, int,
double and at least one udt).
That's very good advice. Wish I had know about that when first
writing template code.
Post by Francis Glassborow
Post by Michaelangelo
9. Extra whitespace never hurts!!
Whitespace that causes wrapping, or expands code so it no longer fits on
the screen does hurt.
For newsgroups posting I agree. i.e. 72 cols or less.

In most cases (95%) I would agree, however, this is still a Rule of
Thumb, not a Mandate.

The whole point in code layout is to group common functionality. I'm
not about to split & limit my code to 72 columns just because someone
else is too lazy to use a 17" + monitor, or a smaller font just
because they can't read ONE function. The point of Rules is know when
they apply in the majority of cases, and when it is "better" to use an
exception.

i.e.

Here are 2 cases, where breaking up the code, only clutters up the
paradigm / operation.

friend void Multiply( Vector4_t & dst, const Vector4_t & lhs, const
Matrix4_t & rhs )
{
// Yes, this could be expressed compactly as
// Vector vTemp(
// Dot( lhs, rhs.GetCol(0) ),
// Dot( lhs, rhs.GetCol(1) ),
// Dot( lhs, rhs.GetCol(2) ),
// Dot( lhs, rhs.GetCol(3) ));
// OPTIMIZATION: This is manually forced inline code,
// avoiding ALL temporaries.
// TODO: Inline with assembly.
dst.x = (lhs.x * rhs.m00) + (lhs.y * rhs.m10) + (lhs.z * rhs.m20) +
(lhs.w * rhs.m30);
dst.y = (lhs.x * rhs.m01) + (lhs.y * rhs.m11) + (lhs.z * rhs.m21) +
(lhs.w * rhs.m31);
dst.z = (lhs.x * rhs.m02) + (lhs.y * rhs.m12) + (lhs.z * rhs.m22) +
(lhs.w * rhs.m32);
dst.w = (lhs.x * rhs.m03) + (lhs.y * rhs.m13) + (lhs.z * rhs.m23) +
(lhs.w * rhs.m33);
}


Here are some good example of where extra whitespace
can help to show the logical structure.
They aren't the best examples, but this is all I could dig up at the
moment.

inline void MakeRotation * ()
{

m00 = _1;/* m01 m02 m03 */
/*m10 */ m11 = c; m12 =-s;
/*m20 */ m21 = s; m22 = c;
/*m30 m31 m32 */ m33 = _1;

// or

m00 = cy * cz; m01 = cz*sxsy-cxsz; m02 = sx*sz+cxcz*sy; /*m03 */
m10 = cy * sz; m11 = cxcz+sxsy*sz; m12 = cxsz*sy-cz*sx; /*m13 */
m20 = - sy ; m21 = cy * sx ; m22 = cx * cy ; /*m23 */
/*m30 m31 m32 */m33 =_1;

}

Fortunately, these cases are rare, but they show up enough, that I've
learnt that this rule is not an absolute. (Surprisingly they only tend
to show up when you have an 'table' or a matrix of numbers/strings.)
Post by Francis Glassborow
Post by Michaelangelo
12. Learn to read (& understand) other people's code -- you'll become
a better writer in the process. Everyone has their own coding style,
but I recommend you seperate your functions via comments, at the very
least.
Code Reading: The Open Source Perspective by Diomidis Spinellis
Haven't heard of that book. Thx for the tip.
Post by Francis Glassborow
Post by Michaelangelo
13. I would recommend a few C++ books.
- The C++ Programming Language (Third Edition), by Stroustrup
- Thinking in C++, by Bruce Eckel
- All books by Scott Meyers, and Andrei Alexandrescu
Andrei's work is a little advanced but those from Herb Sutter should be
in this list
Well, they were relatively sorted from beginner to advanced :-)

Just because a person doesn't understand the syntax and usage of C++
in the more advanced books, doesn't mean it's not worthwhile to learn
about the patterns used.

I know that I enjoyed reading the ARM when I was learning about C++
back in the day. The examples were uncluttered, and to the point. It
helped me in learning about the funky new syntax of C++ ;-)
Post by Francis Glassborow
Post by Michaelangelo
- ARM (although quite old, it is still very usefull)
I think that The Design & Evolution of C++ would be a better choice, the
ARM has too many points of difference with Standard C++ (not surprising
as it was published circa 1989)
I really love D&E. I never thought that "beginner" or "regular" C++
users would benefit from it, but now that I'm re-reading it, I can see
your point.

Thx again for the book tips!

Cheers

--
Original, Fun Palm games by the Lead Designer of Majesty!
http://www.arcanejourneys.com/

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Ali Cehreli
2004-08-11 01:22:11 UTC
Permalink
Unfortunately the following code can't even be compiled. You didn't
post the actual code.
Post by KK
Hello all,
int rows;
int cols;
matType **element;
matrix(){};
matrix(int rows, int cols, matType fillValue=0);
What if 0 can't be a valid value for matType? Change the default value
above to matType():

matrix(/* ... */, fillValue=matType());
Post by KK
matrix(const matrix
&copy);
~matrix();
matrix operator=(matrix A);
operator= ordinarily returns a reference:

matrix & operator=(matrix A);


Also, your actual code must have an operator(), which you must have
forgotten to post:

matType & operator() (int row, int column)
{
return element[row][column];
}

Otherwise the 'B(k,i)=A(i,k);' line below couldn't compile.

(You probably need to have the const version of that too.)
Post by KK
};
template<class matType>
matrix<matType>::matrix (int rows, int cols, matType fillValue){
register int i,k;
this->rows=rows;
this->cols=cols;
Use the member initialization list:

matrix<matType>::matrix (int rows, int cols, matType fillValue)
:
rows(rows),
cols(cols),
element(new matType* [rows])
{

Use different names for the members and the arguments that initialize
them; otherwise it's confusing. For example:

:
rows_(rows),
/* ... */
Post by KK
register int i,k;
Why the 'register' keyword here? It is hopeful but most likely
completely ineffective.

And here is an important problem... Are matrices with different
rows/columns assignable?
Post by KK
matrix<matType> matrix<matType>::operator=(matrix<matType> A) {
int i,k;
rows=A.rows;
cols=A.cols;
for(i=0;i<rows;i++){
for(k=0;k<cols;k++){
element[i][k]=A.element[i][k];
The line above can't work for mismatched sizes of matrices.
Post by KK
void main(){
Just like in C, 'main' returns 'int' in C++. Change it to

int main() {

Ali

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Ben Hutchings
2004-08-11 09:25:15 UTC
Permalink
Subject: Why does this snippet crash?
This snippet won't crash because the Transpose function can't even be
compiled. However, I'll assume that you meant to include the
following additional member function:

template <class matType=int>
matType matrix::operator()(int i, int k)
{
return element[i][k];
}

Given that, the reason the code crashes is in this function:

<snip>
matrix<matType> matrix<matType>::operator=(matrix<matType> A)
{
int i,k;
rows=A.rows;
cols=A.cols;
Since this can change the dimensions of the matrix, you need to
allocate a new buffer of the appropriate dimensions.
for(i=0;i<rows;i++){
for(k=0;k<cols;k++){
element[i][k]=A.element[i][k];
As it is, this will attempt to assign to elements that don't exist.
}
}
return *this;
}
template <class matType=int> class matrix {
int rows;
int cols;
matType **element;
These members should be private. The rows and cols members will
need accessor functions though.
matrix(){};
This fails to initialise the member variables which *must* be
initialised in order for the destructor to work. It cannot safely be
used. Consider whether you really need a default constructor at all.
matrix(int rows, int cols, matType fillValue=0);
matrix(const matrix &copy);
~matrix();
matrix operator=(matrix A);
};
template<class matType>
matrix<matType>::matrix (int rows, int cols, matType fillValue){
register int i,k;
The use of "register" is entirely unecessary; the compiler can do a
better job of register allocation than you and will probably ignore
such hints. Also it is unnecessary to declare variables until they
are needed, and it is inadvisable to do so as it increases the risk of
mistakenly failing to initialise them.
this->rows=rows;
this->cols=cols;
element= new matType* [rows];
Members should normally be initialised through the initialiser-list of
the constructor.
for(i=0;i<rows;i++){
element[i]= new matType [cols];
This raises the risk of a memory leak. If one of the row allocations
fails, the row pointer array and all preceding rows will not be freed.
You can avoid this risk (and simplify initialisation) by creating a
single array of elements:

element = new matType* [rows * cols];

then initialising element[0] to element[rows * cols - 1] and
addressing elements as element[i * cols + k].
for(k=0;k<cols;k++){
element[i][k]=fillValue;
}
}
}
template<class matType>
matrix<matType>::~matrix(){
for(int i=0;i<rows;i++)
delete [] element[i];
delete [] element;
This could then use a single delete[] expression statement.
}
<snip>
template <class matType>
matrix<matType> Transpose(matrix<matType> A)
This signature makes the function inefficient as it requires the
argument matrix to be copied for no good reason. The same is true of
the assignment operator. It is generally better to declare the
parameter with a reference-to-const type instead, as in the copy-
constructor.
{
matrix<matType> B(A.cols,A.rows);
for(register int i=-1;++i<A.rows;)
for(register int k=-1;++k<A.cols;)
B(k,i)=A(i,k);
return B;
}
These loops are inconsistent with your earlier loops and with
idiomatic C++. I suggest you loop from 0 to the limit as normal.
void main(){
main() must return int.
matrix<double> D(3,4,1);
D=Transpose(D);
}
Any suggestions/Corrections in the code is highly appreciated.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
KK
2004-08-11 20:15:39 UTC
Permalink
Post by Ben Hutchings
Subject: Why does this snippet crash?
........
Post by Ben Hutchings
void main(){
main() must return int.
matrix<double> D(3,4,1);
D=Transpose(D);
}
Any suggestions/Corrections in the code is highly appreciated.
Hello All,
I am greatly indebted to above valuable suggestions and advice. I
am learning C++ on my own and dont know the "tricks for trade". I am
following the advice now and trying to build a more robust code. My
special thanks to Michaelangelo for providing excellent review. By the
way, it was not a home problem- I am little too old to be in school :)
Thank you one and all.
KK

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Loading...