Cagri Alp
Cagri Alp

Reputation: 51

Allocating memory in a function and returning it

int** transpose(int** matrix,int row, int column)
{
    int** new_mat = new int*[column];


    for(int i = 0; i < column; i++)
    {
        new_mat[i] = new int[row];
    }
    for(int i  = 0; i < row; i++ )
    {
        for(int j = 0; j < column; j ++)
        {
            new_mat[j][i] = matrix[i][j];
        }
    }

    return new_mat;
}

I have written this function but something feels wrong I couldn't decide whether should I delete new_mat somehow basically function returns this value how should I manage with memory without using any smart pointers or something?

Upvotes: 5

Views: 166

Answers (5)

snake_style
snake_style

Reputation: 1168

If you don't want to use any smart pointers and vectors, then try like this. matrix - is a wrapper for 2D-dynamic array with size [col][row].

#include <iostream>
#include <algorithm>

using namespace std;

class matrix
{
private:
    unsigned int m_row;
    unsigned int m_col;
    int **m_data;

public:    
    matrix(unsigned int row, unsigned int col) : m_row(row), m_col(col), m_data(nullptr)
    {
        alloc();
    }

    matrix(const matrix &m) : m_row(m.get_rows_count()), m_col(m.get_cols_count()), m_data(nullptr)
    {
        alloc();
        for(unsigned int i = 0; i < m_row; i++)
            for(unsigned int j = 0; j < m_col; j++)
                m_data[i][j] = m[i][j];
    }

    ~matrix()
    {
        free();
    }

    unsigned int get_rows_count() const { return m_row; }
    unsigned int get_cols_count() const { return m_col; }
    const int* operator[](unsigned int ind) const
    {
        return m_data[ind];
    }
    int* operator[](unsigned int ind)
    {
        return m_data[ind];
    }

    matrix& operator=(const matrix &m)
    {
        free();
        m_row = m.get_rows_count();
        m_col = m.get_cols_count();
        alloc();
        for(unsigned int i = 0; i < m_row; i++)
            for(unsigned int j = 0; j < m_col; j++)
                m_data[i][j] = m[i][j];
        return *this;
    }

// you need move-operations:
    //matrix(matrix&& other) = delete;       // move constructor (rule of 5)
    //matrix& operator=(matrix&& other);     // move assignment  (rule of 5)

    void print()
    {
        for(unsigned int i = 0; i < m_row; i++)
        {
            for(unsigned int j = 0; j < m_col; j++)
                cout << m_data[i][j] << " ";
            cout << endl;
        }
    }

private:
    void alloc()
    {
        if(m_data)
            return;
        m_data = new int*[m_row];
        for(unsigned int i = 0; i < m_row; i++)
        {
            m_data[i] = new int[m_col];
            std::fill(m_data[i], m_data[i] + m_col, 0);
        }
    }
    void free()
    {
        if(!m_data)
            return;
        for(unsigned int i = 0; i < m_row; i++)
            delete[]m_data[i];
        delete[]m_data;
        m_data = nullptr;
    }
};

matrix transpose(const matrix matrix_in)
{
    unsigned int M = matrix_in.get_rows_count();
    unsigned int N = matrix_in.get_cols_count();
    matrix out(N, M);

    for(unsigned int i = 0; i < M; i++)
        for(unsigned int j = 0; j < N; j++)
            out[j][i] = matrix_in[i][j];
    return out;
}


int main(int argc, char* argv[])
{
    matrix m1(5, 7);
    m1[0][1] = m1[0][2] = m1[0][3] = 7;
    auto m2 = transpose(m1);
    m1.print();
    cout << endl;
    m2.print();
}

In any case, it is better to free the memory in the same place where it was allocated. If you do not want to use some classes, you can do it like this:

void transpose(int **matr_in, int **matr_out, int M, int N)
{
    for(int i = 0; i < M; i++)
        for(int j = 0; j < N; j++)
            matr_out[j][i] = matr_in[i][j];
}

int **create_matrix(int M, int N)
{
    int **m = new int*[M];
    for(int i = 0; i < M; i++)
        m[i] = new int[N];
    return m;
}

void delete_matrix(int **m, int M)
{
    for(int i = 0; i < M; i++)
        delete []m[i];
    delete []m;
}


int main()
{
    int M = 5, N = 4;
    int **m1 = create_matrix(M, N);
// fill matrix m1
    int **m2 = create_matrix(N, M);

    transpose(m1, m2, M, N);

    delete_matrix(m1, M);
    delete_matrix(m2, N);

    return 0;
}

Upvotes: 1

Aconcagua
Aconcagua

Reputation: 25526

You really should think about your matrix representation:

int** matrix = ...; // create matrix of 10x12

// doing quite a lot of stuff

delete[] matrix[7]; // possibly even forgotten -> memory leak
matrix[7] = new int[7];

and you now have a jagged array. Although std::vector will relieve you from all the memory management stuff, you still won't be able to prevent jagged arrays with:

std::vector<std::vector<int>> matrix = ...; // create matrix of 10x12

// doing quite a lot of stuff

matrix[7].resize(7);

Safest thing you can do is create your own class wrapping around the data; I'll be using std::vectors to hold the data, this will make the whole memory management stuff much easier:

template <typename T> // more flexibility: you can use arbitrary data types...
class Matrix          // (but you don't _need_ to make a template from)
{
    std::vector<std::vector<T>> data;
public:
    Matrix(size_t rows, size_t columns)
        : data(rows)
    {
        for(auto& d : data)
            d.resize(columns);
    }
    // the nice thing about using std::vector as data container is
    // that default generated move/copy constructors/assignment
    // operators and destructor are fine already, so you can forget
    // about rule of three or five respectively

    // but you need ways to access your data:
    size_t rows() { return data.size(); }
    size_t columns() { return data.empty() ? 0 : data[0].size(); } 

    ??? operator[](size_t index);
    ??? operator[](size_t index) const;
 };

Well, the index operators... What you actually want to achieve is something that you can access the matrix just like you did with your arrays:

Matrix<int> m(10, 12);
m[7][7] = 7;

But what should we return? A reference to an inner vector would again allow to modify its size and to create a jagged array this way. Solution: A wrapper class around!

template <typename T>
class Matrix
{
    // all we had so far...

    template <typename Data>
    class Row
    {
        Data& data;
        friend class Matrix;

        Row(std::vector<T>& data)
                : data(data)
        { }
    public:
        // default constructed constructors/operators/destructor
        // themselves and being public are fine again...
        auto& operator[](size_t index) { return data[index]; }
    };
    auto operator[](size_t index) { return Row(data[index]); }
    auto operator[](size_t index) const { return Row(data[index]); }
};

Why Row a template? Well, we need different Row types (mutable and immutable access to data) as return types for the two different index operators...

Finally: If you implement yourself, I'd reorder the private/public sections such that public comes first. This improves readability for users of your Matrix class, as they are interested in public interface only (normally) unless they intend to inherit from. But that (currently) is not a good idea here anyway as this class is not intended for, just as std::vector is not either. If you want that: make the destructor virtual:

virtual ~Matrix() = default;

If you feel more comfortable with, you could make the rule of three/five explicit:

Matrix(Matrix const& other) = default;            // rule of three
Matrix& operator=(Matrix const& other) = default; // rule of three
Matrix(Matrix&& other) = default;                 // rule of five
Matrix& operator=(Matrix&& other) = default;      // rule of five

Analogously for Row class. Be aware that if you insist on using raw arrays inside then you will have to write all of these explicitly!

Transposing matrices could then be done via a free function again:

Matrix transpose(Matrix const& m)
{
    Matrix t(m.columns(), m.rows());
    // loops as you had
    return t;
}

You could even provide a member function that transposes the matrix itself, best: use above transpose function:

template <typename T>
class Matrix
{
public:
    void transpose()
    {
        Matrix t(transpose(*this));
        t.data.swap(data); // cleanup of previously owned data done in t's destructor...
    }

Upvotes: 1

Exlife
Exlife

Reputation: 99

you provide a function return a pointer array which holds seperated memory blocks (each represents one row). then you must also provide the free (or delete) function at the same time, and in the same module (to ensure the memory managerment functions matches exactly).

int** transpose(int** matrix, int row, int column)
{
    int** new_mat = new int*[column];
    ...
    return new_mat;
}

//when free the mat, cols is not concerned; 
void free_mat(int** matrix, int rows)
{
     int i;

     for(i= 0; i< rows; i++)
         delete[] matrix[i];

     delete[] matrix;
}

//use the function as:
int** m2 = transpose(m1, rows, cols);
...
free_mat(m2, cols);

//after free_mat(), m2 still holds the address.
//so make it nullptr.

m2 = NULL; 

also you can allcate one plane continuous memory block to present 2-dimention matrix:

int* mat = new int[rows * cols];
//transfer mat[iRow][iCol] to mat[iRow * cols + iCol];
return mat;

Upvotes: 0

Peter
Peter

Reputation: 36597

To answer the question as asked, the caller would need to release the returned pointer. For every usage of operator new in the function, there needs to be a corresponding usage of operator delete in the caller. The caller would do this when the matrix returned is no longer needed i.e. anything that is deleted should not subsequently be used.

A better approach - in many respects, including no potential to forget to release memory - is to avoid using pointers directly, avoid using operator new (or variants) or operator delete directly. Instead, use a standard container, such as std::vector(std::vector<int> >. If used carefully, standard containers manage their own elements, and keep a record of their own size, so there is no possibility of memory leak (when a standard container ceases to exist, any dynamically allocated memory it uses also is released).

In principle, you should be able to simplify your function to something that is declared as

 std::vector<std::vector<int> > transpose(const std::vector<std::vector<int> > &matrix);

rather than needing to pass numbers of rows and columns as separate arguments (the vectors will keep track). I'll leave implementing that function as an exercise, since you'll learn more of use that way.

Upvotes: 1

gsamaras
gsamaras

Reputation: 73366

The caller would use the returned matrix.

Moreover, it could acquire ownership. As a result, when the matrix would be no longer needed, it could delete it.

Another option, is for you, to provide another function, that will delete the matrix. The caller then, must call that function to de-allocate the dynamically allocated memory.


However, smart pointers is a nice C++ feature, and I encourage you to give them a shot.

Furthermore, since this C++, you could use a std::vector<std::vector<int>> for the type of your matrix. That way, you don't have to worry about memory management, since everything about it, will happen automatically.

Upvotes: 1

Related Questions