implicit narrowing in variadic template constructors

m-tosch commented 3 years ago

Background

Orignally, the types were enforced to be of the type that was declared for the Vector/Matrix. Putting different types into the same container throws an error and does not compile e.g.

mu::Vector<3, int> a{4, 5, 6}; // compiles
mu::Vector<3, int> a{4, 5, 6.0F}; // does not compile

However, more and more of the other constructors have been explicitly build to accept a different type than the one that the container was defined for. They apply implicit casting if necessary which is noted in the docstring of those constructors too. Technically, these constructors allow for the type to be different which in their case implies that the type of all values is different. For a variadic template constructor, it may be up to N values that are of a different type. But it should work just about the same.

It makes sense to allow this, since c++ behaves the same way when encountering assignments/initializations/... of variables of different types, e.g.

float f = 1.5F;
int i = f; // implicit cast to 1

For convenience it could be useful to allow code like this:

mu::Vector<3, float> a{4, 5, 6};

but also for consistency between the constructors, different types should be allowed everywhere.

Proposal

remove the std::is_same_v requirement on the variadic template constructors in Vector and Matrix.
move the tests in vector_type.h and matrix_type.h of these constructors to same type combination init tests

m-tosch commented 3 years ago

Instead of the std::is_same, the variadic constructor needs an std::is_arithmetic to make sure that all arguments are arithmetics.

template <typename... TArgs,
    std::enable_if_t<
        sizeof...(TArgs) == N &&
            (std::is_arithmetic_v<std::remove_reference_t<TArgs>> && ...), int> = 0>
Vector(TArgs &&... args) : data_{std::forward<TArgs>(args)...} {}

Compiling the following now valid example:

mu::Vector<3, int> v{1, 2.5F, 3};

gives different outputs for gcc and clang.

gcc

throws a warning

/workspaces/mu/include/mu/vector.h: In instantiation of 'mu::Vector<N, T>::Vector(TArgs&& ...) [with TArgs = {int, float, int}; typename std::enable_if<((sizeof... (TArgs) == N) && (is_arithmetic_v<typename std::remove_reference<_UElements>::type> && ...)), int>::type <anonymous> = 0; long unsigned int N = 3; T = int]':
/workspaces/mu/tests/test_vector2d.cpp:51:22:   required from 'void Vector2DTypeFixture_testTestTestTest_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = mu::Vector2D<int>]'
/workspaces/mu/tests/test_vector2d.cpp:30:1:   required from here
/workspaces/mu/include/mu/vector.h:66:64: warning: narrowing conversion of 'std::forward<float>((* & args#1))' from 'float' to 'int' [-Wnarrowing]
   66 |   Vector(TArgs &&... args) : data_{std::forward<TArgs>(args)...} {}

clang

throws an error

/workspaces/mu/include/mu/vector.h:66:36: error: type 'float' cannot be narrowed to 'int' in initializer list [-Wc++11-narrowing]
  Vector(TArgs &&... args) : data_{std::forward<TArgs>(args)...} {}
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~
/workspaces/mu/tests/test_vector2d.cpp:51:22: note: in instantiation of function template specialization 'mu::Vector<3, int>::Vector<int, float, int, 0>' requested here
  mu::Vector<3, int> v3{1, 2.5F, 3};
                     ^

Two possible approaches (Vector only)

1. static cast

wrap the data conversion in the constructor into a static_cast

Vector(TArgs &&... args) : data{static_cast<T>(std::forward<TArgs>(args))...} {}
                                               ^^^^^^

2. compiler options

Append arguments to the compiler options of gcc and clang to specifically allow implicit narrowing without throwing warnings or error gcc -Wno-narrowing clang -Wno-narrowing

m-tosch commented 3 years ago

Matrix variadic template constructor makes problems still, with either of the two solutions. Throws errors. investigating...

clang

<source>:55:3: note: candidate template ignored: deduced conflicting types for parameter 'TArgs' ('int' vs. 'float')
  Matrix(TArgs const(&&... rows)[M]) : data{to_array(rows)...} {}

gcc

<source>:55:3: note: candidate: 'template<class ... TArgs, typename std::enable_if<((sizeof... (TArgs) == 2) && (is_arithmetic_v<typename std::remove_reference<TArgs>::type> && ...)), int>::type <anonymous> > Matrix<N, M, T>::Matrix(const TArgs (&&)[M]...) [with TArgs = {TArgs ...}; typename std::enable_if<((sizeof... (TArgs) == N) && (is_arithmetic_v<typename std::remove_reference<TArgs>::type> && ...)), int>::type <anonymous> = <anonymous>; long unsigned int N = 2; long unsigned int M = 2; T = int]'
   55 |   Matrix(TArgs const(&&... rows)[M]) : data{to_array(rows)...} {}
      |   ^~~~~~
<source>:55:3: note:   template argument deduction/substitution failed:

m-tosch commented 3 years ago

see stackoverflow question: constructor with nested variadic template arguments of different type arithmetics comment on the questions hints to https://en.cppreference.com/w/cpp/language/list_initialization section "Narrowing conversions". Seems like it's not possible to do int to float or float to int implicitly in list initialization. explicitly it's possible for the constructor in Vector (see above) but the double nested matrix constructor may not be accessible like that for an explicit static_cast.

m-tosch commented 3 years ago

would be possible with the "variant array" and lots of boiler plate code (which is not the issue at all) proposed by the stackoverflow answer. Issue is that a caller would then have to know about this "variant array" and wrap his/her value in it.

Matrix<2,3,int> m{ make_variant_array(1,2,3.5F), make_variant_array(4,5,6)};
// will be [ [1,2,3], [4,5,6] ]

This makes it harder to use the Matrix class in general.

Result

Will not implement implicit narrowing for varidic template constructors in Matrix and Vector. The std::is_same will not be removed on the constructors. For mixed-type input values, a caller should convert his/her values to the same type first, before calling the constructors.

m-tosch / mu