[PATCH v2 01/17] libcamera: matrix: Replace SFINAE with static_asserts

[Barnabás Pőcze]

Hi


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.


Regards,
Barnabás Pőcze


>   {
> +	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++) {