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

[Laurent Pinchart]

On Tue, Apr 01, 2025 at 03:50:41PM +0200, Stefan Klug wrote:
> 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 :-)

Works for me. Whoever uses the constructor first in C++17 will have to
deal with the situation :-)

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