[PATCH v2 02/17] libcamera: matrix: Make most functions constexpr

[Stefan Klug]

On Tue, Apr 01, 2025 at 04:08:24AM +0300, Laurent Pinchart wrote:
> On Tue, Apr 01, 2025 at 03:59:57AM +0300, Laurent Pinchart wrote:
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Mar 19, 2025 at 05:11:07PM +0100, Stefan Klug wrote:
> > > By zero-initializing the data_ member we can make most functions
> > > constexpr which will come in handy in upcoming patches.  Note that this
> > > is due to C++17. In C++20 we will be able to leave data_ unintialized
> > 
> > s/unintialized/uninitialized/
> > 
> > Given that we only operate on small matrices I think this is acceptable.
> > Please add a todo comment to indicate that initialization of data should
> > be removed with C++20.
> > 
> > > for constexpr.  The Matrix(std::array) version of the constructor can
> > > not be constexpr because std::copy only became constexpr in C++20.
> 
> Actually, would open-coding std::copy allow making the constructor
> constexpr ? If subsequent patches should be updated accordingly.

I believe yes. I didn't bother to do that as we don't have any user of
that constructor and it is unclear what comes first. A constexpr usage
of that constructor or us switching to C++20. While I write that: What
about leaving the constexpr here as we have other places where we label
things as constexpr that are actually not constexpr. So the intent would
be clear, just that it doesn't work in C++17 :-)

Best regards,
Stefan

> 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Added this patch
> > > ---
> > >  include/libcamera/internal/matrix.h | 15 +++++++--------
> > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> > > index 8399be583f28..2e7929e9060c 100644
> > > --- a/include/libcamera/internal/matrix.h
> > > +++ b/include/libcamera/internal/matrix.h
> > > @@ -25,9 +25,8 @@ class Matrix
> > >  	static_assert(std::is_arithmetic_v<T>, "Matrix type must be arithmetic");
> > >  
> > >  public:
> > > -	Matrix()
> > > +	constexpr Matrix()
> > >  	{
> > > -		data_.fill(static_cast<T>(0));
> > >  	}
> > >  
> > >  	Matrix(const std::array<T, Rows * Cols> &data)
> > > @@ -35,7 +34,7 @@ public:
> > >  		std::copy(data.begin(), data.end(), data_.begin());
> > >  	}
> > >  
> > > -	static Matrix identity()
> > > +	static constexpr Matrix identity()
> > >  	{
> > >  		Matrix ret;
> > >  		for (size_t i = 0; i < std::min(Rows, Cols); i++)
> > > @@ -63,14 +62,14 @@ public:
> > >  		return out.str();
> > >  	}
> > >  
> > > -	Span<const T, Rows * Cols> data() const { return data_; }
> > > +	constexpr Span<const T, Rows * Cols> data() const { return data_; }
> > >  
> > > -	Span<const T, Cols> operator[](size_t i) const
> > > +	constexpr Span<const T, Cols> operator[](size_t i) const
> > >  	{
> > >  		return Span<const T, Cols>{ &data_.data()[i * Cols], Cols };
> > >  	}
> > >  
> > > -	Span<T, Cols> operator[](size_t i)
> > > +	constexpr Span<T, Cols> operator[](size_t i)
> > >  	{
> > >  		return Span<T, Cols>{ &data_.data()[i * Cols], Cols };
> > >  	}
> > > @@ -85,7 +84,7 @@ public:
> > >  	}
> > >  
> > >  private:
> > > -	std::array<T, Rows * Cols> data_;
> > > +	std::array<T, Rows * Cols> data_{};
> > 
> > We usually write
> > 
> > 	std::array<T, Rows * Cols> data_ = {};
> > 
> > With that and the comment,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > >  };
> > >  
> > >  template<typename T, typename U, unsigned int Rows, unsigned int Cols>
> > > @@ -130,7 +129,7 @@ constexpr Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<
> > >  }
> > >  
> > >  template<typename T, unsigned int Rows, unsigned int Cols>
> > > -Matrix<T, Rows, Cols> operator+(const Matrix<T, Rows, Cols> &m1, const Matrix<T, Rows, Cols> &m2)
> > > +constexpr Matrix<T, Rows, Cols> operator+(const Matrix<T, Rows, Cols> &m1, const Matrix<T, Rows, Cols> &m2)
> > >  {
> > >  	Matrix<T, Rows, Cols> result;
> > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart