3
\$\begingroup\$

I have implemented a Matrix class that allocates memory using unique_ptr. How to make it more diverse and optimized in terms of memory usage and efficiency?

//
// Created by Mayukh Sarkar on 26/12/19.
//

#ifndef DP2_MATRIX_H
#define DP2_MATRIX_H


#include <cstdio>
#include <memory>
#include <algorithm>

template <typename T>
class Matrix {
private:
    size_t row{};
    size_t col{};
    std::unique_ptr<T[]> data;
public:
    explicit Matrix(size_t row, size_t col, T def) {
        // Create a matrix of row X col and initialize
        // each element of the matrix with def
        this->row = row;
        this->col = col;
        this->data = std::make_unique<T[]>(this->row * this->col);
        std::fill_n(data.get(), row*col, def);
    }

    // Overload the [] operator for the 2d array like access
    T* operator[](int r) {
        if (r >= row) {
            throw std::out_of_range("Can not access out of bound element!");
        }
        return &this->data[r*col];
    }

    // Overload the << operator for console logging
    friend std::ostream& operator<< (std::ostream& os, Matrix &mObj) {
        auto shape = mObj.shape();
        for(int i=0; i< shape.first; i++) {
            for(int j=0; j<shape.second; j++) {
                os << mObj[i][j] << " ";
            }
            os << "\n";
        }
        return os;
    }

    // Set all the values of the Matrix
    void setValues(T value) {
        std::fill_n(data.get(), row*col, value);
    }

    // Get row and col values
    std::pair<size_t, size_t> shape() const {
        return std::make_pair(this->row, this->col);
    }
};

#endif //DP2_MATRIX_H

I want to specifically optimize the []operator overloading because I feel it is not good to handle pointers directly. Any suggestions?

\$\endgroup\$
3
\$\begingroup\$

Pragma once

Instead of

#ifndef DP2_MATRIX_H
#define DP2_MATRIX_H

consider using

#pragma once

Once is non-standard, but supported by all compilers supporting C++14 and reduces compilation times.

Includes

On MSVC, #include <stdexcept> is needed for the out_of_range error. I also think your include lines look tidier if you sort them in alphabetical order. :)

Bounds checking

Matrix<int>(10, 10, 0)[10][0]

Is an error that will be detected by your bounds checking, but

Matrix<int>(10, 10, 0)[0][10]

is the same type of error, but won't be detected. Consider following the STL convention described in this Stackoverflow answer. That is, you remove bounds checking from operator[] but add a method at which does bounds checking for both dimensions.

Const correctness

You should add const declared variants of the accessors. You can also add it to the parameters, but imho, that is not as useful.

Result:

#pragma once

#include <algorithm>
#include <cstdio>
#include <memory>
#include <stdexcept>

template <typename T>
class Matrix {
private:
    size_t row{};
    size_t col{};
    std::unique_ptr<T[]> data;
    void boundscheck(int r, int c) const {
        if (!(0 <= r && r < row && 0 <= c && c < col)) {
            throw std::out_of_range("Can not access out of bound element!");
        }
    }
public:
    // Set all the values of the Matrix
    void setValues(T value) {
        std::fill_n(data.get(), row*col, value);
    }
    explicit Matrix(size_t row, size_t col, T def) {
        // Create a matrix of row X col and initialize
        // each element of the matrix with def
        this->row = row;
        this->col = col;
        this->data = std::make_unique<T[]>(this->row * this->col);
        setValues(def);
    }
    // Overload the [] operator for the 2d array like access
    T* operator[](int r) {
        return &this->data[r * col];
    }
    const T* operator[](int r) const {
        return &this->data[r * col];
    }
    T& at(int r, int c) {
        boundscheck(r, c);
        return this->data[r * col + c];
    }
    const T& at(int r, int c) const {
        boundscheck(r, c);
        return this->data[r * col + c];
    }
    // Overload the << operator for console logging
    friend std::ostream& operator<< (std::ostream& os, Matrix &mObj) {
        auto shape = mObj.shape();
        for(int i=0; i< shape.first; i++) {
            for(int j=0; j<shape.second; j++) {
                os << mObj[i][j] << " ";
            }
            os << "\n";
        }
        return os;
    }
    // Get row and col values
    std::pair<size_t, size_t> shape() const {
        return std::make_pair(this->row, this->col);
    }
};

I don't think it is possible to make a general matrix class consume less memory or run quicker than that. Compilers generate pretty good code for the accessor functions. You probably have to use a different data type such as a sparse matrix to improve performance much.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It is really a shame that C++ doesn't have anything that can enforce OutOfBoud access with [] operator or even [][] operator. Anyway nice solutions. \$\endgroup\$ – Mayukh Sarkar Dec 26 '19 at 19:15
  • \$\begingroup\$ @MayukhSarkar For that, you use .at(). Also, .operator()() is common for multi-dimensional indexing. \$\endgroup\$ – Deduplicator Dec 27 '19 at 16:51
  • \$\begingroup\$ @Deduplictor Yeah but a [][] operator would have been nice. Wonder how boost does it? \$\endgroup\$ – Mayukh Sarkar Dec 27 '19 at 17:08
  • \$\begingroup\$ @MayukhSarkar You can fake it using nested classes. But imo, you shouldn't do it. It makes the code harder to understand for no tangible benefit. \$\endgroup\$ – Björn Lindqvist Dec 27 '19 at 17:25
  • 1
    \$\begingroup\$ You should make input for operator[] and function at of type size_t - so on definition it is clear that you don't accept negative values. It will also fix a few casting warnings and some when user misuses operator []. \$\endgroup\$ – ALX23z Dec 28 '19 at 20:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.