[v3,01/16] libcamera: matrix: Replace SFINAE with static_asserts
diff mbox series

Message ID 20250403154925.382973-2-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug April 3, 2025, 3:49 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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
---
 include/libcamera/internal/matrix.h | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

Comments

Kieran Bingham May 2, 2025, 7:38 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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?



> ---
>  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
>
Barnabás Pőcze May 2, 2025, 1:58 p.m. UTC | #2 
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@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
>>
Kieran Bingham May 3, 2025, 6:52 p.m. UTC | #3 
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@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
> >>
>
Paul Elder May 7, 2025, 3:31 p.m. UTC | #4 
On Thu, Apr 03, 2025 at 05:49:06PM +0200, Stefan Klug wrote:
> 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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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
> ---
>  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
>
Barnabás Pőcze May 8, 2025, 6:39 a.m. UTC | #5 
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@ideasonboard.com>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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
>>>>
>>

Patch
diff mbox series 

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