C++ new statement. Help me

Discussion in 'Mac Programming' started by quanganhct, Apr 16, 2011.

  1. macrumors member

    Joined:
    Dec 29, 2010
    #1
    Hi,
    Apparently I write a little code which use "new" inside of my function, but it appear that it do not work well.
    Code:
    #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.
     
  2. jiminaus, Apr 17, 2011
    Last edited: Apr 17, 2011

    macrumors 65816

    jiminaus

    Joined:
    Dec 16, 2010
    Location:
    Sydney
    #2
    You need extra level of indirection for M in inputMatrix.
    Code:
    void inputMatrix(double [color=red]*[/color]**M,int& line,int& col){
    	cout << "Input number of lines and columns : ";
    	cin>>line>>col;
    	
    	[color=red](*M)[/color]=new double*[line];
    	for (int i=0; i<line; i++) {
    		[color=red](*M)[/color][i]=new double[col];
    	}
    	
    	cout << "Input your matrix :"<<endl;
    	for (int i=0; i<line; i++)
    		for (int j=0; j<col; j++) cin>>[color=red](*M)[/color][i][j];
    	
    }
    
    And then in main
    Code:
    	inputMatrix([color=red]&[/color]M1,line1,col1);
    

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

    Code:
    [color=red]double **[/color]inputMatrix(int& line,int& col){
    	cout << "Input number of lines and columns : ";
    	cin>>line>>col;
    	
    	[color=red]double **[/color]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];
    	
            [color=red]return M;[/color]
    }
    
    And then in main:
    Code:
    	[color=red]M1=[/color]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.
    Code:
    #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 which can be downloaded from http://www.robertnz.net/download.html.
     
  3. macrumors G5

    gnasher729

    Joined:
    Nov 25, 2005
    #3
    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.
     
  4. macrumors 6502

    Joined:
    Apr 24, 2008
    #4
    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?
     
  5. macrumors 65816

    jiminaus

    Joined:
    Dec 16, 2010
    Location:
    Sydney
    #5
    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.
     
  6. thread starter macrumors member

    Joined:
    Dec 29, 2010
    #6
    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.
     
  7. macrumors 6502a

    Joined:
    Jun 27, 2010
    #7
    Always use a single dimension array for rectangular multidimensional arrays. It is so much easier, faster and error-free.
     
  8. macrumors 6502

    Joined:
    Apr 24, 2008
    #8
    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" :)
     
  9. thread starter macrumors member

    Joined:
    Dec 29, 2010
    #9
    OK, here I go.
    Code:
    /*
     *  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 :)
     
  10. macrumors member

    Joined:
    Apr 8, 2007
    Location:
    UK
    #10
    Using more than 2 levels of indirection is poor practice and asking for trouble. It's difficult to read and error prone.
     
  11. macrumors 65816

    jiminaus

    Joined:
    Dec 16, 2010
    Location:
    Sydney
    #11
    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.


    quanganhct you should refactor 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.

    Code:
    #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
     
  12. thread starter macrumors member

    Joined:
    Dec 29, 2010
    #12
    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:
     
  13. macrumors 65816

    jiminaus

    Joined:
    Dec 16, 2010
    Location:
    Sydney
    #13
    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.
     
  14. thread starter macrumors member

    Joined:
    Dec 29, 2010
    #14
    Code:
        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
     
  15. macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #15
    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
     
  16. thread starter macrumors member

    Joined:
    Dec 29, 2010
    #16
    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 :
    Code:
    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
     
  17. macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #17
    "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.

    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:
    Code:
    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
     
  18. macrumors 65816

    jiminaus

    Joined:
    Dec 16, 2010
    Location:
    Sydney
    #18
    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.
     

Share This Page