View Full Version : C++ new statement. Help me

quanganhct

Apr 16, 2011, 11:55 PM

Hi,

Apparently I write a little code which use "new" inside of my function, but it appear that it do not work well.

#include "iostream"

#include "cstdlib"

#include "iomanip"

using namespace std;

void inputMatrix(double **M,int& line,int& col){

cout << "Input number of lines and columns : ";

cin>>line>>col;

M=new double*[line];

for (int i=0; i<line; i++) {

M[i]=new double[col];

}

cout << "Input your matrix :"<<endl;

for (int i=0; i<line; i++)

for (int j=0; j<col; j++) cin>>M[i][j];

}

void killMatrix(double **M,int line,int col){

cout<<"DD";

for (int i=0; i<line; i++) delete[] M[i];

delete[] M;

}

int main(){

int line1,col1,line2,col2;

double **M1,**M2,**result;

inputMatrix(M1,line1,col1);

cout<<line1<<" "<<col1<<endl;

cout<<M1[0][1]<<endl;

// inputMatrix(M2,line2,col2);

// cout<<line2<<" "<<col2<<endl;

// result=add2Matrix(M1,M2,line1,col1,line2,col2);

// outputMatrix(result,line1,col1);

killMatrix(M1,line1,col1);

// killMatrix(M2,line2,col2);

// killMatrix(result,line1,col1);

return 0;

}

and it return "Segmentation fault"

I guess "new" do not work properly.

Does anyone have any ideas ?

Thanks in advance.

jiminaus

Apr 17, 2011, 02:04 AM

You need extra level of indirection for M in inputMatrix.

void inputMatrix(double ***M,int& line,int& col){

cout << "Input number of lines and columns : ";

cin>>line>>col;

(*M)=new double*[line];

for (int i=0; i<line; i++) {

(*M)[i]=new double[col];

}

cout << "Input your matrix :"<<endl;

for (int i=0; i<line; i++)

for (int j=0; j<col; j++) cin>>(*M)[i][j];

}

And then in main

inputMatrix(&M1,line1,col1);

Or alternatively, don't use an out-style parameter for M, instead pass it back as the result.

double **inputMatrix(int& line,int& col){

cout << "Input number of lines and columns : ";

cin>>line>>col;

double **M=new double*[line];

for (int i=0; i<line; i++) {

M[i]=new double[col];

}

cout << "Input your matrix :"<<endl;

for (int i=0; i<line; i++)

for (int j=0; j<col; j++) cin>>M[i][j];

return M;

}

And then in main:

M1=inputMatrix(line1,col1);

But (assuming you're just using C++ as a better C) you should create a matrix struct that binds the pointer, col, and line together.

#include <iostream>

#include <cstdlib>

#include <iomanip>

using namespace std;

struct matrix {

double **values;

int line;

int col;

};

matrix inputMatrix(){

matrix m;

cout << "Input number of lines and columns : ";

cin>>m.line>>m.col;

m.values =new double*[m.line];

for (int i=0; i<m.line; i++) {

m.values[i]=new double[m.col];

}

cout << "Input your matrix :"<<endl;

for (int i=0; i<m.line; i++)

for (int j=0; j<m.col; j++) cin>>m.values[i][j];

return m;

}

void outputMatrix(const matrix &m) {

for (int i=0; i<m.line; i++) {

for (int j=0; j<m.col; j++)

cout << m.values[i][j] << ' ';

cout << endl;

}

}

void killMatrix(matrix &m){

for (int i=0; i<m.line; i++) delete[] m.values[i];

delete[] m.values;

m.values = NULL;

}

int main(){

matrix M1, M2, result;

M1 = inputMatrix();

// cout<<m1.line<<" "<<m1.col<<endl;

// cout<<m1.values[0][1]<<endl;

// M2 = inputMatrix();

// cout<<m2.line<<" "<<m2.col<<endl;

// result=add2Matrix(M1,M2);

outputMatrix(M1);

killMatrix(M1);

// killMatrix(M2);

// killMatrix(result);

return 0;

}

But, surely there are ready-made, tested and optimised matrix implementations out there. For example, there's Robert Davies' Newmat C++ matrix library (http://www.robertnz.net/nm_intro.htm) which can be downloaded from http://www.robertnz.net/download.html.

gnasher729

Apr 17, 2011, 02:19 AM

Hi,

Apparently I write a little code which use "new" inside of my function, but it appear that it do not work well.

In that function, where do you use the value M that is passed in? You just overwrite its value. That looks very, very suspicious. The value of M1 never gets defined.

Sander

Apr 17, 2011, 06:39 AM

But (assuming you're just using C++ as a better C) you should create a matrix struct that binds the pointer, col, and line together.

Hear hear. I see this all the time, but a program doesn't progress from C to C++ by changing its file extension from .c to .cpp and changing malloc/free into new/delete. I guess C++ being compatible with C is both a blessing and a curse...

To the OP: I assume you are writing this matrix stuff as a learning exercise. It's a popular exercise to learn OO. You should make your matrix class "self-contained", and have it clean up after itself. You will run into a bunch of problems on the way: How do you copy matrices? Do you copy its contents? Do you hand out references instead? Who is then responsible for cleaning it up, and how do you prevent other code from accessing zombie matrices?

jiminaus

Apr 17, 2011, 07:03 AM

Hear hear. I see this all the time, but a program doesn't progress from C to C++ by changing its file extension from .c to .cpp and changing malloc/free into new/delete. I guess C++ being compatible with C is both a blessing and a curse...

Actually it does precisely by changing .c to .cpp or gcc to g++. I can't remember in which book I learnt this phrase, but "C++ as a better C" is (or was before C99) a legitimate usage of C++. A C++ program does not have to be object-oriented to be a legitimate use of C++.

If this is an OOP exercise, than the OP is totally failing. Actually I picked it up as being a FORTRAN port. But maybe that was just because of the by-reference function arguments.

quanganhct

Apr 17, 2011, 08:16 AM

well in fact I haven't learnt about OPP yet. I wrote this program 'cause I just had a lesson about Matrix.

anyway, thanks alot :). I'll test it out and report later.

SidBala

Apr 17, 2011, 02:01 PM

Always use a single dimension array for rectangular multidimensional arrays. It is so much easier, faster and error-free.

Sander

Apr 18, 2011, 02:34 AM

Actually it does precisely by changing .c to .cpp or gcc to g++. I can't remember in which book I learnt this phrase, but "C++ as a better C" is (or was before C99) a legitimate usage of C++. A C++ program does not have to be object-oriented to be a legitimate use of C++.

Of course you are right, jiminaus. Perhaps I should have written "good C++ program" or (to be more politically correct) "idiomatic C++ program".

What I was trying to get at is that when people struggle with a problem in C which is easier solved in C++, it's not going to be by substituting "new" for "malloc" :)

quanganhct

Apr 18, 2011, 10:20 AM

OK, here I go.

/*

* matrix.cpp

*

*

* Created by NGUYEN Quang Anh on 4/17/11.

* Copyright 2011 __MyCompanyName__. All rights reserved.

*

*/

#include "iostream"

#include "cstdlib"

#include "iomanip"

#include "exception"

using namespace std;

struct matrix {

double **value;

int line;

int col;

};

matrix inputMatrix(int& line,int& col){

cout << "Input number of lines and columns : ";

cin>>line>>col;

matrix M;

M.value=new double*[line];

for (int i=0; i<line; i++) {

M.value[i]=new double[col];

}

cout << "Input your matrix :"<<endl;

for (int i=0; i<line; i++)

for (int j=0; j<col; j++) cin>>M.value[i][j];

M.line=line;

M.col=col;

return M;

}

void killMatrix(matrix M){

for (int i=0; i<M.line; i++) delete[] M.value[i];

delete[] M.value;

}

void outputMatrix(matrix M){

for (int i=0; i<M.line; i++) {

for (int j=0; j<M.col; j++) cout << setw(3) << M.value[i][j] ;

cout << endl;

}

}

matrix add2Matrix(matrix M1,matrix M2){

bool notCorrect=((M1.line != M2.line)||(M1.col != M2.col));

if (notCorrect) throw 10;

matrix result;

result.line=M1.line;

result.col=M1.col;

result.value=new double*[result.line];

for (int i=0; i<result.line; i++) {

result.value[i]=new double[result.col];

}

for (int i=0; i<result.line; i++)

for (int j=0; j<result.col; j++)

result.value[i][j]=M1.value[i][j]+M2.value[i][j];

return result;

}

matrix multi2Matrix(matrix M1,matrix M2){

bool notCorrect=(M1.col != M2.line);

if (notCorrect) throw 10;

matrix result;

result.line=M1.line;

result.col=M2.col;

result.value=new double*[result.line];

for (int i=0; i<result.line; i++) {

result.value[i]=new double[result.col];

}

for (int i=0; i<result.line; i++) {

for (int j=0; j<result.col; j++) {

result.value[i][j]=0;

for (int k=0; k<M1.col; k++) result.value[i][j]+=M1.value[i][k]*M2.value[k][j];

}

}

return result;

}

int main(){

int line,col;

matrix M1,M2,result;

M1=inputMatrix(line,col);

M2=inputMatrix(line,col);

try {

result=multi2Matrix(M1,M2);

cout << "Your Result :"<< endl;

outputMatrix(result);

killMatrix(M1);

killMatrix(M2);

killMatrix(result);

}

catch (int i){

if (i==10) cerr << "Cannot execute operation" <<endl;

killMatrix(M1);

killMatrix(M2);

}

return 0;

}

It went pretty well. Thanks a lot guys :) Now I try to write a function calculate matrix inverse :)

flashy-cat

Apr 18, 2011, 01:49 PM

Using more than 2 levels of indirection is poor practice and asking for trouble. It's difficult to read and error prone.

jiminaus

Apr 18, 2011, 05:55 PM

Using more than 2 levels of indirection is poor practice and asking for trouble. It's difficult to read and error prone.

Agreed, I had to diagram it out. Actually I wanted to pass a reference to a the double pointer, but I couldn't get the syntax for that. I didn't try very hard, because the out-style 2d array parameter wasn't a good idea to begin with.

OK, here I go.

It went pretty well. Thanks a lot guys :) Now I try to write a function calculate matrix inverse :)

quanganhct you should refactor (http://en.wikipedia.org/wiki/Code_refactoring) your code by isolating out the code that allocates a matrix.

Let me show you how your example would look in object-oriented C++ that sandar was advocating.

#include <iostream>

#include <cstdlib>

#include <iomanip>

#include <exception>

using namespace std;

class matrix {

public:

static matrix inputMatrix();

matrix(int aLine, int aCol); // Constructor

matrix(const matrix& aSrcM); // Copy constructor

~matrix(); // Destructor

int line() const { return theLine; }

int col() const { return theCol; }

double get(int aLine, int aCol) const;

void set(int aLine, int aCol, double aValue);

private:

double** theValue;

int theLine;

int theCol;

};

matrix operator +(const matrix& M1, const matrix& M2);

matrix operator *(const matrix& M1, const matrix& M2);

ostream& operator <<(ostream& os, const matrix& M);

matrix::matrix(int aLine, int aCol)

{

// TODO: Bounds-check aLine and aCol

theLine = aLine;

theCol = aCol;

theValue = new double*[theLine];

for (int i=0; i<theLine; i++) {

theValue[i] = new double[theCol];

}

}

matrix::matrix(const matrix& M)

{

theLine = M.line();

theCol = M.col();

theValue = new double *[theLine];

for (int i=0; i<theLine; i++) {

theValue[i] = new double[theCol];

// TODO: Replace inner loop with memcpy

for (int j=0; j<theCol; j++) {

set(i, j, M.get(i, j));

}

}

}

matrix::~matrix()

{

for (int i=0; i<theLine; i++) delete[] theValue[i];

delete[] theValue;

}

matrix matrix::inputMatrix()

{

int line, col;

cout << "Input number of lines and columns : ";

cin >> line >> col;

// TODO: Check line and col are valid

matrix M(line, col);

cout << "Input your matrix :"<<endl;

double v;

for (int i=0; i<line; i++) {

for (int j=0; j<col; j++) {

cin >> v;

M.set(i, j, v);

}

}

return M;

}

ostream& operator <<(ostream& os, const matrix& M)

{

for (int i=0; i<M.line(); i++) {

for (int j=0; j<M.col(); j++) os << setw(3) << M.get(i,j);

os << endl;

}

return os;

}

matrix operator +(const matrix& M1, const matrix& M2)

{

bool notCorrect=((M1.line() != M2.line())||(M1.col() != M2.col()));

// TODO: Replace throwing an int with throwing a std::exception

// or a custom subcalss of it

if (notCorrect) throw 10;

matrix result(M1.line(), M1.col());

for (int i=0; i<result.line(); i++)

for (int j=0; j<result.col(); j++)

result.set(i, j, M1.get(i,j)+M2.get(i,j));

return result;

}

matrix operator *(const matrix& M1, const matrix& M2)

{

bool notCorrect=(M1.col != M2.line);

// TODO: Replace throwing an int with throwing a std::exception

// or a custom subcalss of it

if (notCorrect) throw 10;

matrix result(M1.line(), M2.col());

for (int i=0; i<result.line(); i++) {

for (int j=0; j<result.col(); j++) {

result.set(i,j,0);

for (int k=0; k<M1.col(); k++) {

result.set(i, j, result.get(i,j) + M1.get(i,k)*M2.get(k,j));

}

}

}

return result;

}

int main()

{

matrix M1 = matrix::inputMatrix();

matrix M2 = matrix::inputMatrix();

try {

matrix result = M1 * M2;

cout << "Your Result :" << endl

<< result;

// result will be automatically destructed by compiler at this point

}

// TODO: Replace throwing an int with throwing a std::exception

// or a custom subcalss of it

catch (int i){

if (i==10) cerr << "Cannot execute operation" <<endl;

}

return 0;

// M1 and M2 will be automatically destructed by compiler at this point

}

It's been a while since I've coded C++. It's nice to feel the raw power in my fingers again. :D

quanganhct

Apr 19, 2011, 11:14 AM

Using more than 2 levels of indirection is poor practice and asking for trouble. It's difficult to read and error prone.

It seems I ask silly question, but hey, what is "level of indirection", and "where did i use more than 2 levels of indirection ?

you mean 2 dimension array ? :confused:

jiminaus

Apr 19, 2011, 03:33 PM

It seems I ask silly question, but hey, what is "level of indirection", and "where did i use more than 2 levels of indirection ?

you mean 2 dimension array ? :confused:

Levels of indirection is the number pointers you need to "hop" to get to the value itself.

In C, the number of stars in a type is the level of indirection. So int* has 1 level of indirection to the int, int** has 2 levels of indirection to the int, int*** had 3 level of indirection to the int, etc.

You didn't use more than 2 levels of indirection. You do use 2 levels of indirection for the 2 dimensional array.

The first solution I posted for inputMatrix increased the levels of indirection to 3, so that it could pass back the pointer to the 2 dimensional array which it allocates inside itself. The 2 dimensional array is double**, so a pointer to that is double***, which has 3 levels of indirection.

quanganhct

Apr 20, 2011, 08:33 AM

double get(int aLine, int aCol) const;

void set(int aLine, int aCol, double aValue);

Could you please explain this 2 functions to me ? I didn't find where you defined these 2 functions.

Thanks alot

lee1210

Apr 20, 2011, 08:49 AM

double get(int aLine, int aCol) const;

void set(int aLine, int aCol, double aValue);

Could you please explain this 2 functions to me ? I didn't find where you defined these 2 functions.

Thanks alot

It doesn't look like those were included, but surely you can work out what the implementation should be? The data store in matrix is theValue, which is a double**. Surely given a line and column you could work out what double should be returned, and given a line, column, and double value you can work out what to set to that double value. In both cases you'd want to compare to theLine and theCol to make sure you're not over or understepping a dimension, but that should be pretty easy.

-Lee

quanganhct

Apr 20, 2011, 09:13 AM

It doesn't look like those were included, but surely you can work out what the implementation should be? The data store in matrix is theValue, which is a double**. Surely given a line and column you could work out what double should be returned, and given a line, column, and double value you can work out what to set to that double value. In both cases you'd want to compare to theLine and theCol to make sure you're not over or understepping a dimension, but that should be pretty easy.

-Lee

Well, i ask that question, 'cause I don't know for sure if he missed the definition of those 2 functions. I did do some searching on internet before asking, and I found out that "get" and "set" are key-words. And if those "get" and "set" are in this context , then I'm beat, and I need some explanations.

Oh ya, and another question :

matrix::matrix(const matrix& M)

{

theLine = M.line();

theCol = M.col();

theValue = new double *[theLine];

for (int i=0; i<theLine; i++) {

theValue[i] = new double[theCol];

// TODO: Replace inner loop with memcpy

for (int j=0; j<theCol; j++) {

set(i, j, M.get(i, j));

}

}

}

What does that comment mean ? 'cause when I search for "memcpy", there aren't anything like your syntax

http://www.cplusplus.com/reference/clibrary/cstring/memcpy/

Sorry for asking lot of questions :) It can't be help since I'm a totally newbie. I just learned about C++ 6 months ago, and since I want to learn it myself, alot of new things appear in front of me and I'm speechless lol :d

lee1210

Apr 20, 2011, 10:19 AM

Well, i ask that question, 'cause I don't know for sure if he missed the definition of those 2 functions. I did do some searching on internet before asking, and I found out that "get" and "set" are key-words. And if those "get" and "set" are in this context , then I'm beat, and I need some explanations.

"get" and "set" are not C++ keywords. Where did you read that? Wherever it is, either you misunderstood so you should re-read, or it is wrong. There is a STL container called "set", but this isn't a reserved word.

Oh ya, and another question :

matrix::matrix(const matrix& M)

{

theLine = M.line();

theCol = M.col();

theValue = new double *[theLine];

for (int i=0; i<theLine; i++) {

theValue[i] = new double[theCol];

// TODO: Replace inner loop with memcpy

for (int j=0; j<theCol; j++) {

set(i, j, M.get(i, j));

}

}

}

What does that comment mean ? 'cause when I search for "memcpy", there aren't anything like your syntax

It wouldn't be like this syntax, the comment is that this syntax should be replaced with that usage instead. Instead of looping and using get to grab a double at a time, you'd have, say, a double *getRow(int rowNum) method that would return the address of one row, then you'd:

double *sourceRow = M.getRow(i);

memcpy(theValue[i],sourceRow,theCol*sizeof(double));

This would be much faster than looping to grab a bunch of contiguous bytes of memory sizeof(double) bytes at a time, then sticking that value somewhere else over and over again. Using memcpy takes advantage of our data all being contiguous in memory and allows a faster copy in larger chunks than a single double at a time.

-Lee

jiminaus

Apr 20, 2011, 03:35 PM

double get(int aLine, int aCol) const;

void set(int aLine, int aCol, double aValue);

Could you please explain this 2 functions to me ? I didn't find where you defined these 2 functions.

Thanks alot

Sorry, I did forget them. They were defined inline in the class at one stage. I removed them from there intending to defined them later with TODO comments about bounds checking the parameters.