5
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?

4
  • 5
    Use a vector<vector<int>> and move it when returning. Commented Dec 5, 2018 at 8:39
  • 4
    The same rules apply regardless of where memory gets allocated. If you deallocate it and then use it, the behaviour is undefined. (The proper solution is to use std::vector and not worry about it.) Commented Dec 5, 2018 at 8:44
  • 10
    @George "Move-returning" a local variable may hinder optimisation. Don't try to out-smart the compiler. Commented Dec 5, 2018 at 8:48
  • 1
    Actually, as this is working code, code review would be the more appropriate site to post this question. Commented Dec 5, 2018 at 10:14

5 Answers 5

1

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.

Sign up to request clarification or add additional context in comments.

Comments

1

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.

Comments

1

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...
    }

Comments

1

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;
}

10 Comments

Needs to handle transpose(m1,m1,M,N);
@SaileshD only for square matrices
Agree on the class (although it yet requires better encapsulation), not on proposed non-class variant, it is more error prone than original version (the latter determined size from matrix passed to, in the former, the user is required to get the matter right...). In given case, would just document that transpose_matrix returns a new one which needs to be cleaned up via delete_matrix just like one created by create_matrix.
Broken Rule of 5/3: implement copy/move constructor.
@Aconcagua Of course, you are right, it's just a sample.
|
0

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;

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.