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

[Kieran Bingham]

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 ?

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