| Message ID | 20250319161152.63625-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. > > Signed-off-by: Stefan Klug <stefan.klug@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@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; >
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. > > Signed-off-by: Stefan Klug <stefan.klug@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@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; > >
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@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@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
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@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@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; > > > >
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_{}; }; 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;
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 for constexpr. The Matrix(std::array) version of the constructor can not be constexpr because std::copy only became constexpr in C++20. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v2: - Added this patch --- include/libcamera/internal/matrix.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)