Search code examples
c++constantsside-effects

side effects on constant variable of nested array type


I encountered strange side effects that I cannot explain to myself in the slightest. Probably I am missing something very obvious, but I have looked for the bug for several hours now and the code is quite simple, so I concluded that I must have some rather fundamental misunderstanding about... something.

Consider this code, that was meant to compute the product of two 2d matrices (I have changed the set() function to add -1 to the argument cell, to make the debug output more comprehensible.

template<class T, unsigned column_count, unsigned row_count>
class Matrix
{
private:
    const static unsigned row_length    = column_count;
    const static unsigned column_length = row_count;
    using matrix_type = std::array<std::array<T, row_length>, row_count>;
    matrix_type matrix;

public:
    using value_type = T;

    Matrix(const matrix_type& matrix) : matrix(matrix) {}
    Matrix() {}

    friend std::ostream& operator<<(std::ostream& o, const Matrix& rhs)
    {
        for (unsigned i = 0; i < column_count; ++i) {
            for (unsigned j = 0; j < row_count; ++j) {
                o << rhs.matrix[i][j] << ' ';
            }
            o << '\n';
        }
        return o;
    }

    const auto& get_rows() const { return matrix; }

    const auto get_columns() const
    {
        std::array<std::array<T, column_length>, column_count> columns;

        for (unsigned i = 0; i < row_length; ++i) {
            for (unsigned j = 0; j < column_length; ++j) {
                columns[i][j] = matrix[j][i];
            }
        }
        return columns;
    }

    void set(unsigned i, unsigned j, T v) { matrix[i][j] = -1; }

    friend Matrix operator*(const Matrix& m1, const Matrix& m2)
    {

        auto columns = m1.get_columns();
        auto rows    = m2.get_rows();

        Matrix m3;

        std::cout << "before:"
                  << "\n";
        std::cout << m1 << "\n";
        std::cout << m2 << "\n";
        std::cout << m3 << "\n";

        unsigned i{ 0 };

        for (const auto& row : rows) {

            i++;
            unsigned j{ 0 };

            for (const auto& column : columns) {

                j++;
                value_type v{ 0 };

                for (unsigned k = 0; k < column.size(); ++k) {
                    v += row[k] * column[k];
                }
                m3.set(i, j, v);
            }
        }

        std::cout << "after:"
                  << "\n";
        std::cout << m1 << "\n";
        std::cout << m2 << "\n";
        std::cout << m3 << "\n";

        return m3;
    }
};

As you can see, the getter functions either return a copy or a constant reference. The operator* function takes constant parameters.

I now construct two matrices like so:

std::array<int, 3> c1{ { 1, 2, 3 } };
std::array<int, 3> c2{ { 4, 5, 6 } };
std::array<int, 3> c3{ { 7, 8, 9 } };

std::array<std::array<int, 3>, 3> m1{ { c1, c2, c3 } };

std::array<std::array<int, 3>, 3> m2 = m1;

Matrix<int, 3, 3> matrix1(m1);
Matrix<int, 3, 3> matrix2(m2);

Now I invoke operator* in different ways:

matrix1* matrix2;

result:

before:
1 2 3
4 5 6
7 8 9

1 2 3
4 5 6
7 8 9

0 0 0
0 0 0
0 0 183238709

after:
-1 -1 -1
4 5 6
7 8 9

1 2 3
4 5 6
7 8 9

0 0 0
0 -1 -1
-1 -1 -1

matrix2* matrix1;

result:

before:
1 2 3
4 5 6
7 8 9

1 2 3
4 5 6
7 8 9

0 0 0
0 0 0
0 0 -1823473620

after:
1 2 3
4 5 6
7 8 9

-1 -1 -1
4 5 6
7 8 9

0 0 0
0 -1 -1
-1 -1 -1

matrix1* matrix1;

result:

before:
1 2 3
4 5 6
7 8 9

1 2 3
4 5 6
7 8 9

1385085408 32767 401515081
1 1385085440 32767
1385085440 32767 1385085464

after:
-1 -1 -1
4 5 6
7 8 9

-1 -1 -1
4 5 6
7 8 9

1385085408 32767 401515081
1 -1 -1
-1 -1 -1

As you can see, the matrix that gets passed as first argument will be changed. Which makes no sense to me, as it is passed as a const and set() only operates on m3. Somehow m3 gets partly "bound" to the matrix that is the first argument of operator*. Why?


Solution

  • You're writing out of bounds in your loops, since both i and j are incremented too early.

    The code should look like this:

    for (const auto& row : rows) {
        // i++;
        unsigned j{ 0 };
    
        for (const auto& column : columns) {
            // j++;
            value_type v{ 0 };
    
            for (unsigned k = 0; k < column.size(); ++k) {
                v += row[k] * column[k];
            }
            m3.set(i, j, v);
    
            j++; // <--
        }
    
        i++; // <--
    }