2

GCC and MSVC throw a compilation error on a method of matrices multiplication with templates, while Clang compiles this successfully without any errors, I cannot understand why. Can someone suggest what is wrong?

I suppose that problem with instantiating operator*(matrix, matrix).

Notes: (error: redefinition of function template, C2995: function template has already defined) [declaration and definition in one .hpp, there are header guards]

I tested this on gcc 7.2.0 and higher and MSVC 19.14.26428.1 and higher. Also I use C++17 standard.

Problematic method:

template <std::size_t Rows_lhs, std::size_t Columns_lhs,
          std::size_t Rows_rhs, std::size_t Columns_rhs>
friend constexpr matrix<value_type, Rows_lhs, Columns_rhs> operator*(
    const matrix<value_type, Rows_lhs, Columns_lhs>& lhs,
    const matrix<value_type, Rows_rhs, Columns_rhs>& rhs)
{
    static_assert(Columns_lhs == Rows_rhs, "Incorrect matrix for product!");

    matrix<value_type, Rows_lhs, Columns_rhs> result{};
    container<value_type, Rows_rhs> thatColumn{};

    for (size_type j = 0; j < Columns_rhs; ++j)
    {
        for (size_type k = 0; k < Rows_rhs; ++k)
        {
            thatColumn.at(k) = rhs(k, j);
        }

        for (size_type i = 0; i < Rows_lhs; ++i)
        {
            const auto thisRow = lhs(i);
            value_type summand{};
            for (size_type k = 0; k < Rows_rhs; ++k)
            {
                summand += thisRow.at(k) * thatColumn.at(k);
            }
            result(i, j) = summand;
        }
    }
    return result;
}

Full code:

#include <iostream>
#include <string>
#include <cmath>
#include <iterator>
#include <algorithm>
#include <array>
#include <initializer_list>


namespace vv
{
template <class Type = double, std::size_t Rows = 1, std::size_t Columns = 1>
class matrix
{
public:
    using value_type                    = Type;
    using size_type                     = std::size_t;

    template <class Type = value_type, std::size_t N = Rows>
    using container                     = std::array<Type, N>;
    using row_container                 = container<value_type, Columns>;
    using row_container_reference       = container<value_type, Columns>&;    
    using const_row_container_reference = const container<value_type, Columns>&;

    using reference                     = value_type&;
    using const_reference               = const value_type&;

    using std_matrix                    = matrix<value_type, Rows, Columns>;


    static constexpr value_type EPS = static_cast<value_type>(1e-10);

    static_assert(std::is_arithmetic_v<value_type>, "Matrix elements type has to be arithmetic!");
    static_assert(Rows > 0 && Columns > 0, "Incorrect size parameters!");

    constexpr matrix() = default;

    constexpr matrix(const std::initializer_list<value_type> list)
    : _data()
    {
        size_type row_counter = 0;
        size_type col_counter = 0;
        for (const auto elem : list)
        {
            _data.at(row_counter).at(col_counter) = elem;
            ++col_counter;
            if (row_counter == Rows && col_counter == Columns)
            {
                break;
            }
            if (col_counter == Columns)
            {
                col_counter = 0;
                ++row_counter;
            }
        }
    }

    std::string get_dimension() const noexcept
    {
        return std::to_string(Rows) + std::string("x")
                + std::to_string(Columns);
    }

    constexpr const_reference operator()(const size_type i, const size_type j) const
    {
        return _data.at(i).at(j);
    }

    constexpr reference operator()(const size_type i, const size_type j)
    { 
        return _data.at(i).at(j);
    }

    constexpr const_row_container_reference& operator()(const size_type i) const
    {
        return _data.at(i);
    }

    constexpr row_container_reference& operator()(const size_type i)
    {
        return _data.at(i);
    }

    constexpr std_matrix& operator*=(const value_type num) noexcept
    {
        for (auto& row : _data)
        {
            for (auto& elem : row)
            {
                elem *= num;
            }
        }
        return *this;
    }

    friend constexpr std_matrix operator*(const std_matrix& mat, const value_type num) noexcept
    {
        std_matrix temp(mat);
        return (temp *= num);
    }

    friend constexpr std_matrix operator*(const value_type num, const std_matrix& mat) noexcept
    {
        return (mat * num);
    }

    friend std::ostream& operator<<(std::ostream& os, const std_matrix& mat)
    {
        os << "[" << mat.get_dimension() << "]\n";
        for (const auto& row : mat._data)
        {
            std::copy(std::begin(row), std::end(row),
                      std::ostream_iterator<value_type>(os, " "));
            os << '\n';
        }
        return os;
    }

    // PROBLEMS HERE BEGIN
    template <std::size_t Rows_lhs, std::size_t Columns_lhs,
              std::size_t Rows_rhs, std::size_t Columns_rhs>
    friend constexpr matrix<value_type, Rows_lhs, Columns_rhs> operator*(
        const matrix<value_type, Rows_lhs, Columns_lhs>& lhs,
        const matrix<value_type, Rows_rhs, Columns_rhs>& rhs)
    {
        static_assert(Columns_lhs == Rows_rhs, "Incorrect matrix for product!");

        matrix<value_type, Rows_lhs, Columns_rhs> result{};
        container<value_type, Rows_rhs> thatColumn{};

        for (size_type j = 0; j < Columns_rhs; ++j)
        {
            for (size_type k = 0; k < Rows_rhs; ++k)
            {
                thatColumn.at(k) = rhs(k, j);
            }

            for (size_type i = 0; i < Rows_lhs; ++i)
            {
                const auto thisRow = lhs(i);
                value_type summand{};
                for (size_type k = 0; k < Rows_rhs; ++k)
                {
                    summand += thisRow.at(k) * thatColumn.at(k);
                }
                result(i, j) = summand;
            }
        }
        return result;
    }
    // END

private:
    container<container<value_type, Columns>, Rows> _data;
};

} // namespace vv


int main()
{
    constexpr vv::matrix<double, 2, 1> a{ 1.0, 2.0 };
    constexpr vv::matrix<double, 1, 2> b{ 4.0, 3.0 };

    constexpr auto c = a * b; // This code occurs error.
    std::cout << c;
    return 0;
}
Vasily Vasilyev
  • 112
  • 2
  • 9
  • 1
    Post code here. –  May 05 '18 at 19:53
  • *GCC and MSVC* -- Version information is missing from your post. – PaulMcKenzie May 05 '18 at 19:56
  • @PaulMcKenzie If not specified, assume current versions. It's easily verifiable with online compilers that it indeed fails with those. –  May 05 '18 at 19:58
  • @PaulMcKenzie, I tested this on gcc 7.2.0 and higher and MSVC 15.6 and higher. – Vasily Vasilyev May 05 '18 at 20:01
  • *"Each identifier that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation"* So user code should not use identifiers such as `_Rows_lhs`. – user7860670 May 05 '18 at 20:03
  • @NeilButterworth, I'd have to show nearly 450 lines of code to reproduce the error. Is this really needed? – Vasily Vasilyev May 05 '18 at 20:05
  • 1
    Not really, you should throw away code that is not relevant to the given problem. So the result is a *minimal* example reproducing stated error. – user7860670 May 05 '18 at 20:06
  • 1
    @VasilyVasilyev -- *It is needed to post nearly 450 lines of code to reproduce the error* -- If the error has to do with function definitions, you could start by removing all of the code within the function bodies, as they are irrelevant to the error in question. If the function requires a `return` then return some dummy value. – PaulMcKenzie May 05 '18 at 20:08
  • A [mcve] is supposed to be **Minimal**. That doesn't mean that you dump all your code here. Take the time to figure out what parts are causing the problem and isolate them. We are not here to do your debugging work for you. – super May 05 '18 at 20:22
  • @PaulMcKenzie, Ok, I have found the same situation but with nearly 200 lines of code) – Vasily Vasilyev May 05 '18 at 20:23
  • @super, This code works in Clang perfectly without any errors. And I guarantee that there are no errors in the algorithms. There are some errors with template instantiating. – Vasily Vasilyev May 05 '18 at 20:26
  • @VasilyVasilyev That's irrelevant. The code produces compilation errors in GCC and MSVC. The question is what part of the code is causing the problem. When you know that and still can't figure out **why** then you have a good question for SO. – super May 05 '18 at 20:28
  • @super, I have removed all redundant code yet. And I wrote that problem in instantiate matrices multiplication method. – Vasily Vasilyev May 05 '18 at 20:31
  • Not related to your problem, but you ought to mention the C++ standard. This doesn't work on c++11 or c++14, but it does on c++17. ("doesn't work" -> even more errors) – John Perry May 05 '18 at 20:45
  • 1
    @JohnPerry, Thanks, I forgot to mention it. Fixed. – Vasily Vasilyev May 05 '18 at 20:49
  • @super, I rewrote my post. – Vasily Vasilyev May 05 '18 at 22:10

1 Answers1

3

The problem is that operator*() is defined inside the template matrix class.

So, when you define a matrix object, say matrix<double, 1, 2>, this function is defined; when you define another object with the same type and different dimensions, say matrix<double, 2, 1>, the exact same template function is redefined.

It seems to me that there isn't nothing that require that function to be friend of matrix so -- suggestion -- delete it inside the class and rewrite it, outside, as follows

template <class Type, std::size_t N>
using container = std::array<Type, N>;

using size_type                     = std::size_t;

template <typename value_type, std::size_t Rows_lhs, std::size_t Columns_lhs,
          std::size_t Rows_rhs, std::size_t Columns_rhs>
constexpr matrix<value_type, Rows_lhs, Columns_rhs> operator*(
    const matrix<value_type, Rows_lhs, Columns_lhs>& lhs,
    const matrix<value_type, Rows_rhs, Columns_rhs>& rhs)
{
    static_assert(Columns_lhs == Rows_rhs, "Incorrect matrix for product!");

    matrix<value_type, Rows_lhs, Columns_rhs> result{};
    container<value_type, Rows_rhs> thatColumn{};

    for (size_type j = 0; j < Columns_rhs; ++j)
    {
        for (size_type k = 0; k < Rows_rhs; ++k)
        {
            thatColumn.at(k) = rhs(k, j);
        }

        for (size_type i = 0; i < Rows_lhs; ++i)
        {
            const auto thisRow = lhs(i);
            value_type summand{};
            for (size_type k = 0; k < Rows_rhs; ++k)
            {
                summand += thisRow.at(k) * thatColumn.at(k);
            }
            result(i, j) = summand;
        }
    }
    return result;
}

If you really want, you can maintain it friend but only declaring it inside the class matrix

template <typename value_type, std::size_t Rows_lhs, 
          std::size_t Columns_lhs, std::size_t Rows_rhs, 
          std::size_t Columns_rhs>
friend constexpr matrix<value_type, Rows_lhs, Columns_rhs> operator*(
    const matrix<value_type, Rows_lhs, Columns_lhs>& lhs,
    const matrix<value_type, Rows_rhs, Columns_rhs>& rhs);

Bonus (off topic) suggestion: there is no need to define four matrix dimension and impose with a static_assert() that the second (Columns_lhs) and the third (Rows_rsh) are equal.

You can unify they in a single template parameter (midDim, in the following example)

template <typename value_type, std::size_t Rows_lhs, std::size_t midDim,
          std::size_t Columns_rhs>
constexpr matrix<value_type, Rows_lhs, Columns_rhs> operator*(
    const matrix<value_type, Rows_lhs, midDim>& lhs,
    const matrix<value_type, midDim, Columns_rhs>& rhs)
{
    matrix<value_type, Rows_lhs, Columns_rhs> result{};
    container<value_type, midDim> thatColumn{};

    for (size_type j = 0; j < Columns_rhs; ++j)
    {
        for (size_type k = 0; k < midDim; ++k)
        {
            thatColumn.at(k) = rhs(k, j);
        }

        for (size_type i = 0; i < Rows_lhs; ++i)
        {
            const auto thisRow = lhs(i);
            value_type summand{};
            for (size_type k = 0; k < midDim; ++k)
            {
                summand += thisRow.at(k) * thatColumn.at(k);
            }
            result(i, j) = summand;
        }
    }
    return result;
}
max66
  • 65,235
  • 10
  • 71
  • 111
  • Huh? `std_matrix` (part of `operator*`'s parameters) is `matrix`, so wouldn't you get a different `operator*` for each template instantiation? (Edit: oh wait, there are multiple `operator*` overloads. Your answer is right.) –  May 05 '18 at 20:53
  • @max66, Thanks a lot! But what magic does Clang use to make this error disappear? – Vasily Vasilyev May 05 '18 at 21:00
  • @hvd - ehmmm.. I don't see `std_matrix` parameters for the `friend` `operator*()` defined inside `matrix`; if I'm not wrong, the signature of `operator*()` depends from `value_type` but not from `Rows` and `Columns`; so (as usual: if I'm not wrong) it's safe to define differents object of different `matrix` types with different `value_type`'s but if you define two `matrix` object with the same `value_type` and different dimensions, you redefine the same operator. – max66 May 05 '18 at 21:00
  • @VasilyVasilyev - the lack of error in clang surprises me too. – max66 May 05 '18 at 21:01
  • @max66 The full code, linked separately in the question, has other overloads that confused me. –  May 05 '18 at 21:03
  • 1
    @VasilyVasilyev - I've simplified the problem and asked [another question](https://stackoverflow.com/questions/50194159/g-and-clang-different-behaviour-with-friend-template-function-defined-inside#50194159) hoping someone can say us who's right between clang++ and g++ – max66 May 05 '18 at 21:33