[PATCH v3 01/16] libcamera: matrix: Replace SFINAE with static_asserts

[Barnabás Pőcze]

Hi


2025. 05. 02. 9:38 keltezéssel, Kieran Bingham írta:
> Quoting Stefan Klug (2025-04-03 16:49:06)
>> 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>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> ---
>>
>> Changes in v2:
>> - Added this patch
>>
>> Changes in v3:
>> - Left SFINAE in place for the operators as static asserts could cause
>>    issues there
> 
> Barnabás, did this update resolve your concerns from v2?

Yes, although as I have mentioned, I think it was fine as is because it
is an internal thing, and it can be changed arbitrarily if needed.


Regards,
Barnabás Pőcze

> 
> 
> 
>> ---
>>   include/libcamera/internal/matrix.h | 19 +++++--------------
>>   1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
>> index a055e6926c94..b9c3d41ef855 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");
>> +
>>   public:
>>          Matrix()
>>          {
>> @@ -123,16 +120,10 @@ Matrix<U, Rows, Cols> operator*(const Matrix<U, Rows, Cols> &m, T d)
>>          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++) {
>> -- 
>> 2.43.0
>>