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

[Barnabás Pőcze]

2025. 05. 03. 20:52 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-05-02 14:58:13)
>> 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.
> 
> Does that mean this patch can have your RB tag ?

Reviewed-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>


> 
> --
> Kieran
> 
> 
>>
>>
>> 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
>>>>
>>