| Message ID | 20250319161152.63625-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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++) {
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"); + 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) { + 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++) {
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> --- Changes in v2: - Added this patch --- include/libcamera/internal/matrix.h | 42 ++++++++--------------------- 1 file changed, 11 insertions(+), 31 deletions(-)