[PATCH v2 01/17] libcamera: matrix: Replace SFINAE with static_asserts
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 1 02:55:13 CEST 2025
On Wed, Mar 19, 2025 at 05:56:20PM +0100, Barnabás Pőcze wrote:
> 2025. 03. 19. 17:11 keltezéssel, Stefan Klug írta:
> > SFINAE is difficult to read and not needed in these cases. Replace it
> > with static_asserts. The idea came from [1] where it is stated:
> >
> > "The use of enable_if seems misguided to me. SFINAE is useful for the
> > situation where we consider multiple candidates for something (overloads
> > or class template specializations) and try to choose the correct one,
> > without causing compilation to fail."
> >
> > [1]: https://stackoverflow.com/questions/62109526/c-friend-template-that-use-sfinae
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Added this patch
> > ---
> > include/libcamera/internal/matrix.h | 42 ++++++++---------------------
> > 1 file changed, 11 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> > index a055e6926c94..8399be583f28 100644
> > --- a/include/libcamera/internal/matrix.h
> > +++ b/include/libcamera/internal/matrix.h
> > @@ -19,14 +19,11 @@ namespace libcamera {
> >
> > LOG_DECLARE_CATEGORY(Matrix)
> >
> > -#ifndef __DOXYGEN__
> > -template<typename T, unsigned int Rows, unsigned int Cols,
> > - std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > -#else
> > template<typename T, unsigned int Rows, unsigned int Cols>
> > -#endif /* __DOXYGEN__ */
> > class Matrix
> > {
> > + static_assert(std::is_arithmetic_v<T>, "Matrix type must be arithmetic");
>
> Here I agree with this change since there are no template specializations, etc.
> You could also add `Rows > 0` and `Cols > 0` potentially. ANd I think the same
> should be done with `Vector` as well.
>
> > +
> > public:
> > Matrix()
> > {
> > @@ -78,13 +75,10 @@ public:
> > return Span<T, Cols>{ &data_.data()[i * Cols], Cols };
> > }
> >
> > -#ifndef __DOXYGEN__
> > - template<typename U, std::enable_if_t<std::is_arithmetic_v<U>>>
> > -#else
> > template<typename U>
> > -#endif /* __DOXYGEN__ */
> > - Matrix<T, Rows, Cols> &operator*=(U d)
> > + constexpr Matrix<T, Rows, Cols> &operator*=(U d)
> > {
> > + static_assert(std::is_arithmetic_v<U>, "Multiplier must be arithmetic");
> > for (unsigned int i = 0; i < Rows * Cols; i++)
> > data_[i] *= d;
> > return *this;
> > @@ -94,14 +88,10 @@ private:
> > std::array<T, Rows * Cols> data_;
> > };
> >
> > -#ifndef __DOXYGEN__
> > -template<typename T, typename U, unsigned int Rows, unsigned int Cols,
> > - std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > -#else
> > template<typename T, typename U, unsigned int Rows, unsigned int Cols>
> > -#endif /* __DOXYGEN__ */
> > -Matrix<U, Rows, Cols> operator*(T d, const Matrix<U, Rows, Cols> &m)
> > +constexpr Matrix<U, Rows, Cols> operator*(T d, const Matrix<U, Rows, Cols> &m)
>
> But here, where this change concerns free function operators, I am not sure,
> here multiple overloads existing does not seem that far-fetched of an idea.
> But since this is an internal type and no such use case exists at the moment
> I think it is fine, just wanted to mention it.
Sample code that exhibits the issue:
--------
#include <iostream>
#include <type_traits>
template<typename T>
T foo(T a, double b)
{
static_assert(std::is_same_v<T, int>, "Must be int");
return a * b;
}
template<typename T>
T foo(T a, T b)
{
return a / b;
}
int main()
{
int a = foo(3, 2.0);
double b = foo(3.0, 2.0);
std::cout << a << " " << b << std::endl;
return 0;
}
--------
$ g++ -W -Wall -o sfinae2 sfinae2.cpp
sfinae2.cpp: In function ‘int main()’:
sfinae2.cpp:20:23: error: call of overloaded ‘foo(double, double)’ is ambiguous
20 | double b = foo(3.0, 2.0);
| ~~~^~~~~~~~~~
sfinae2.cpp:5:3: note: candidate: ‘T foo(T, double) [with T = double]’
5 | T foo(T a, double b)
| ^~~
sfinae2.cpp:12:3: note: candidate: ‘T foo(T, T) [with T = double]’
12 | T foo(T a, T b)
| ^~~
$
--------
#include <iostream>
#include <type_traits>
template<typename T, std::enable_if_t<std::is_same_v<T, int>> * = nullptr>
T foo(T a, double b)
{
return a * b;
}
template<typename T>
T foo(T a, T b)
{
return a / b;
}
int main()
{
int a = foo(3, 2.0);
double b = foo(3.0, 2.0);
std::cout << a << " " << b << std::endl;
return 0;
}
--------
$ g++ -W -Wall -o sfinae2 sfinae2.cpp
$
I'd keep SFINAE for the operators. For the removal of SFINAE for the
Matrix class itself,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > {
> > + static_assert(std::is_arithmetic_v<T>, "Multiplier must be arithmetic");
> > Matrix<U, Rows, Cols> result;
> >
> > for (unsigned int i = 0; i < Rows; i++) {
> > @@ -112,27 +102,17 @@ Matrix<U, Rows, Cols> operator*(T d, const Matrix<U, Rows, Cols> &m)
> > return result;
> > }
> >
> > -#ifndef __DOXYGEN__
> > -template<typename T, typename U, unsigned int Rows, unsigned int Cols,
> > - std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > -#else
> > template<typename T, typename U, unsigned int Rows, unsigned int Cols>
> > -#endif /* __DOXYGEN__ */
> > -Matrix<U, Rows, Cols> operator*(const Matrix<U, Rows, Cols> &m, T d)
> > +constexpr Matrix<U, Rows, Cols> operator*(const Matrix<U, Rows, Cols> &m, T d)
> > {
> > + static_assert(std::is_arithmetic_v<T>, "Multiplier must be arithmetic");
> > return d * m;
> > }
> >
> > -#ifndef __DOXYGEN__
> > -template<typename T,
> > - unsigned int R1, unsigned int C1,
> > - unsigned int R2, unsigned int C2,
> > - std::enable_if_t<C1 == R2> * = nullptr>
> > -#else
> > -template<typename T, unsigned int R1, unsigned int C1, unsigned int R2, unsigned in C2>
> > -#endif /* __DOXYGEN__ */
> > -Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<T, R2, C2> &m2)
> > +template<typename T, unsigned int R1, unsigned int C1, unsigned int R2, unsigned int C2>
> > +constexpr Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<T, R2, C2> &m2)
> > {
> > + static_assert(C1 == R2, "Matrix dimensions must match for multiplication");
> > Matrix<T, R1, C2> result;
> >
> > for (unsigned int i = 0; i < R1; i++) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list