4
\$\begingroup\$

I created a simple 4x4 matrix class (column-major). I would like it to be efficient and to use C++14's full capabilities. Can I improve it?

#include <array>

class mat4
{
public:
    constexpr mat4() noexcept : matrix() {}

    constexpr mat4(const std::array<float, 16> &m) noexcept : matrix(m) {}
    constexpr mat4(std::array<float, 16> &&m) noexcept : matrix(std::move(m)) {}

    constexpr mat4(const mat4 &other) noexcept : matrix(other.matrix) {}
    constexpr mat4(mat4 &&other) noexcept : matrix(std::move(other.matrix)) {}

    constexpr bool operator==(const mat4 &other) const noexcept
    {
        for (size_t i = 0; i < 16; ++i) {
            if ((*this)[i] != other[i]) {
                return false;
            }
        }

        return true;
    }

    constexpr bool operator!=(const mat4 &other) const noexcept
    {
        return !(this->operator==(other));
    }

    constexpr mat4& operator+=(const mat4 &other) noexcept
    {
        for (size_t i = 0; i < 16; ++i) {
            (*this)[i] += other[i];
        }
        return *this;
    }

    constexpr mat4& operator*=(float scalar) noexcept 
    {
        for (size_t i = 0; i < 16; ++i) {
            (*this)[i] *= scalar;
        }
        return *this;
    }

    mat4& operator=(mat4 other) noexcept
    {
        std::swap(this->matrix, other.matrix);
        return *this;
    }   

    constexpr float& operator[](size_t index) { return const_cast<float&>(static_cast<const std::array<float, 16>&>(matrix)[index]); }
    constexpr float operator[](size_t index) const { return matrix[index]; }

    void print() const noexcept
    {
        printf("\n");
        printf("[%.2f][%.2f][%.2f][%.2f]\n", matrix[0], matrix[4], matrix[8], matrix[12]);
        printf("[%.2f][%.2f][%.2f][%.2f]\n", matrix[1], matrix[5], matrix[9], matrix[13]);
        printf("[%.2f][%.2f][%.2f][%.2f]\n", matrix[2], matrix[6], matrix[10], matrix[14]);
        printf("[%.2f][%.2f][%.2f][%.2f]\n", matrix[3], matrix[7], matrix[11], matrix[15]);
    }

private:
    std::array<float, 16> matrix;
};

constexpr const mat4 operator+(mat4 lhs, const mat4 &rhs) noexcept
{
    lhs += rhs;
    return lhs;
}

constexpr const mat4 operator*(const mat4 &lhs, const mat4 &rhs) noexcept
{
    mat4 result;

    for (size_t i = 0; i < 4; ++i) 
    {
        for (size_t j = 0; j < 4; ++j)
        {
            for (size_t k = 0; k < 4; ++k) {
                result[i + 4 * j] += lhs[i + 4 * k] * rhs[k + 4 * j];
            }
        }
    }

    return result;
}

constexpr const mat4 operator*(mat4 lhs, float scalar) noexcept 
{
    lhs *= scalar;
    return lhs;
}

You can test it in this live demo here.

\$\endgroup\$
  • 1
    \$\begingroup\$ @OlzhasZhumabek It compiles under g++ (although I haven't called the functions). I 'm assuming it's differentiating by one function being const and the other not. \$\endgroup\$ – forsvarir Aug 1 '16 at 14:07
  • \$\begingroup\$ @forsvarir, thanks. Missed the const, which is part of the function signature. \$\endgroup\$ – Incomputable Aug 1 '16 at 14:19
2
\$\begingroup\$

I'm not sure how I feel about you hardcoding that this should be a 4x4 matrix of floats, and that it should be column major. It seems straightforward enough to make a templated class like so

template <typename T, size_t numRows, size_t numCols>
class Matrix {
...
};

using mat4 = Matrix<float, 4, 4>;

It might also be worthwhile to have a boolean template parameter for whether or not it should be row or column major; in my experience many applications assume something is row major, and interfacing with your application might yield some unexpected issues.

If you do so, then I think adding a constructor to switch row/column major would be worthwhile as well.

I personally like using operator() instead of operator[] for getting a value at a given row and column; I feel like in general it shouldn't be up to the user to manually calculate the 1d index. Something along the lines of this:

constexpr const T& operator()(size_t row, size_t col) const noexcept 
{
    return matrix[col * numCols + row];
}

constexpr T& operator()(size_t row, size_t col) noexcept
{
    return matrix[col * numCols + row];
}

I also generally assume that at() (or similar) will be provided if indexing (or operator()-like access) is provided.

Don't use a member function like print(); instead implement operator<< so people can use it with their own streams as desired. I see you're using printf - why? I've definitely used it before in my programs, but I usually think of it as a code-smell. If you want to add printf support that's fine, but I would argue that streams should be prioritized.

Your operator== is unnecessarily complicated - std::array implements it.

I don't like using this-> to refer to member functions; just use them. Then your operator!= becomes return !operator==(other).

operator+= and operator*= can be done using std::transform.

You should implement iterators for your matrix as well. Whether you make them row-major or column-major shouldn't matter, but be consistent (assuming you make it possible to switch between orientations). No matter the orientation, the iteration order should be the same.

You should add some improved matrix multiplication algorithms (or, ideally, use a library that does it for you). With some SFINAE wizardry you can probably make a vectorized version, a tiled version, etc. Probably only necessary if you actually make it possible to use larger matrices, although vectorization will probably still be a bit faster even for a small one.

\$\endgroup\$
  • \$\begingroup\$ Great answer! The reason why I only considered designing a 4d column-major square matrix class is because I'm only using matrices in OpenGL but I'll definitely switch to a template as I'm going to need 3d matrices too. As for operator== of std::array and std::transform, I've already thought on using them but they are not constexpr functions and you cannot call them within another constexpr function. I also gave up on the idea of using them because they do not seem to be really faster than a basic for loops (for std::transform at least when I looked up its implementation). \$\endgroup\$ – Corvinus Aug 5 '16 at 16:27
  • 1
    \$\begingroup\$ @Corvinus They won't necessarily be faster, but they're easier to read imo. If they're not constexpr that's a dealbreaker though, unless you think you could implement your own version that is. \$\endgroup\$ – Dannnno Aug 5 '16 at 16:32
0
\$\begingroup\$

This is an attempt to answer my own question. I would be grateful if anyone could comment on it weither they agree/do not agree with parts of my solution.


  • The first argument of type mat4 should be passed by reference rather than by value in operator+, operator* (scalar version) and operator= signatures.

With the help of this reference let's carry out an analysis of the function operator+ (like it is done in this article) to understand why (analysis for operator* and operator= is similar).

Fist, we can assume two things on the code above :

  1. When the conditions for copy elision are met, the compiler will omit copy or move constructors of mat4. I have tested this under GCC 6.1 and Clang 3.8 and it is indeed the case in all the situations I've tested when using the operators of this class.
  2. For this class mat4, that default constructor >= copy constructor = move constructor (the greater the faster in terms of execution speed). Because std::array is an aggregate, moving a float is not any faster than copying it. Copy/move constructor probably use something like memcpy internally and the default constructor memset (because of the call matrix() in mat4's constructor) to set all elements to 0 (which I doubt would be slower than memcpy and may be even faster).

There are two cases to consider when calling operator+ :

  1. The first argument of type mat4 passed to the function is a lvalue.
  2. It is a rvalue.

If the first argument is an lvalue, the pass-by-value (current) version of operator+ will call the copy constructor of mat4 to copy that object into lhs. Then when the function returns, it will call the move constructor of mat4 to move lhs into the object returned by operator+. The reason why it is the move and not the copy constructor that is called is explained in the reference given above. Here is the interesting extract :

(since C++11) In a return statement or a throw-expression, if the compiler cannot perform copy elision but the conditions for copy elision are met or would be met, except that the source is a function parameter, the compiler will attempt to use the move constructor even if the object is designated by an lvalue; see return statement for details.

However, if the argument passed to operator+ is an rvalue, copy of that value into lhs will be elided. In this case only the move constructor is called.

Now let's do the exact same analysis for the pass-by-reference version of operator+ which I propose the implementation to be the following :

constexpr const mat4 operator+(const mat4 &lhs, const mat4 &rhs) noexcept
{
    mat4 result;
    for (size_t i = 0; i < 16; ++i) {
        result[i] = lhs[i] + rhs[i];
    }
    return result;
}

In both cases, i.e. when the first argument to the function is either an lvalue or an rvalue, calling the function will only result in a single call of the default constructor (mat4 result;). Note that the temporary object result will not be moved nor copied into the function's returned value because the conditions for copy elision are met.

Conclusion : this last solution is undeniably faster. Be careful, it is not everytime the case and only true here because of how operator+ can be implemented and because of the two assumptions, especially the second one, i.e. default constructor >= copy constructor = move constructor (note that this conclusion would also be true if default constructor = copy constructor = move constructor).


  • In operator=, it is unnecessary to use std::swap because you do unnecessary copies.

    Just use this->matrix = other.matrix;. std::swap would only be interesting to use if the class implemented a resource (See here).

  • Until complete constexpr support for std::array (C++17), you could create your own array wrapper and so you wouldn't have to write that ugly cast in operator[]

Here is a wrapper inspired from std::array that could work :

template<class _Ty, size_t _Size>
struct arr
{
    constexpr _Ty& operator[](size_t index) noexcept
    {
        return _arr[index];
    }
    constexpr const _Ty& operator[](size_t index) const noexcept
    {
        return _arr[index];
    }

    constexpr _Ty *data() noexcept
    {
        return _arr;
    }

    constexpr const _Ty *data() const noexcept
    {   
        return _arr;
    }

    float _arr[_Size];
};
\$\endgroup\$

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.