| Message ID | 20260609155043.1121340-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart (2026-06-09 16:50:43) > It was envisioned that the Matrix::data_ initializer could be dropped > once moving to C++20. Unfortunately older compilers don't agree. Update > the comment accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/matrix.h | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h > index 0f72263f2536..4900dcc0a419 100644 > --- a/include/libcamera/internal/matrix.h > +++ b/include/libcamera/internal/matrix.h > @@ -117,9 +117,11 @@ public: > > private: > /* > - * \todo The initializer is only necessary for the constructor to be > - * constexpr in C++17. Remove the initializer as soon as we are on > - * C++20. > + * \todo The initializer should be only necessary for the constructor to > + * be constexpr in C++17. However, older gcc versions (at least 9 and > + * 12, but apparently not 10), as well as clang versions (at least 14 > + * and 19), throw compilation errors, even with C++20. Remove the > + * initializer once we can drop support of those old compiler versions. > */ > std::array<T, Rows * Cols> data_ = {}; > }; > > base-commit: 5701eb5f47cf3649e2746a2082c7bb3e0a0577e8 > -- > Regards, > > Laurent Pinchart >
2026. 06. 09. 17:50 keltezéssel, Laurent Pinchart írta: > It was envisioned that the Matrix::data_ initializer could be dropped > once moving to C++20. Unfortunately older compilers don't agree. Update > the comment accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- The `Vector` and `Matrix` classes should be the same in this respect, but they are not. My suggestion is the following: diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h index 0f72263f25..f0d726bc75 100644 --- a/include/libcamera/internal/matrix.h +++ b/include/libcamera/internal/matrix.h @@ -32,9 +32,7 @@ class Matrix static_assert(std::is_arithmetic_v<T>, "Matrix type must be arithmetic"); public: - constexpr Matrix() - { - } + Matrix() = default; Matrix(const std::array<T, Rows * Cols> &data) { @@ -48,7 +46,7 @@ public: static constexpr Matrix identity() { - Matrix ret; + Matrix ret = {}; for (size_t i = 0; i < std::min(Rows, Cols); i++) ret[i][i] = static_cast<T>(1); return ret; @@ -116,12 +114,7 @@ public: } private: - /* - * \todo The initializer is only necessary for the constructor to be - * constexpr in C++17. Remove the initializer as soon as we are on - * C++20. - */ - std::array<T, Rows * Cols> data_ = {}; + std::array<T, Rows * Cols> data_; }; #ifndef __DOXYGEN__ The above change should pass the CI tests with a small change to `test/matrix.cpp`: Matrix<double, 3, 3> m1 = {}; as the test assumes the matrix is zero initialized. Or maybe we should just not allow the creation of uninitialized Vector/Matrix in the first place. > include/libcamera/internal/matrix.h | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h > index 0f72263f2536..4900dcc0a419 100644 > --- a/include/libcamera/internal/matrix.h > +++ b/include/libcamera/internal/matrix.h > @@ -117,9 +117,11 @@ public: > > private: > /* > - * \todo The initializer is only necessary for the constructor to be > - * constexpr in C++17. Remove the initializer as soon as we are on > - * C++20. > + * \todo The initializer should be only necessary for the constructor to > + * be constexpr in C++17. However, older gcc versions (at least 9 and > + * 12, but apparently not 10), as well as clang versions (at least 14 > + * and 19), throw compilation errors, even with C++20. Remove the > + * initializer once we can drop support of those old compiler versions. > */ > std::array<T, Rows * Cols> data_ = {}; > }; > > base-commit: 5701eb5f47cf3649e2746a2082c7bb3e0a0577e8
On Mon, Jun 15, 2026 at 03:52:47PM +0200, Barnabás Pőcze wrote: > 2026. 06. 09. 17:50 keltezéssel, Laurent Pinchart írta: > > It was envisioned that the Matrix::data_ initializer could be dropped > > once moving to C++20. Unfortunately older compilers don't agree. Update > > the comment accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > The `Vector` and `Matrix` classes should be the same in this respect, Agreed. > but they are not. My suggestion is the following: > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h > index 0f72263f25..f0d726bc75 100644 > --- a/include/libcamera/internal/matrix.h > +++ b/include/libcamera/internal/matrix.h > @@ -32,9 +32,7 @@ class Matrix > static_assert(std::is_arithmetic_v<T>, "Matrix type must be arithmetic"); > > public: > - constexpr Matrix() > - { > - } > + Matrix() = default; Could you explain why you think we should drop constexpr for the default constructor ? Not that I object, but I'd like to record the correct rationale in the commit message. > > Matrix(const std::array<T, Rows * Cols> &data) > { > @@ -48,7 +46,7 @@ public: > > static constexpr Matrix identity() > { > - Matrix ret; > + Matrix ret = {}; > for (size_t i = 0; i < std::min(Rows, Cols); i++) > ret[i][i] = static_cast<T>(1); > return ret; > @@ -116,12 +114,7 @@ public: > } > > private: > - /* > - * \todo The initializer is only necessary for the constructor to be > - * constexpr in C++17. Remove the initializer as soon as we are on > - * C++20. > - */ > - std::array<T, Rows * Cols> data_ = {}; > + std::array<T, Rows * Cols> data_; > }; > > #ifndef __DOXYGEN__ > > > The above change should pass the CI tests with a small change to `test/matrix.cpp`: > > Matrix<double, 3, 3> m1 = {}; > > as the test assumes the matrix is zero initialized. That passes all CI tests, as well as my local build tests. > Or maybe we should just not allow the creation of uninitialized Vector/Matrix > in the first place. For small matrices it probebly won't make much difference. For larger ones, skipping initialization can improve efficiency, at the cost of possible usage of unused values of course. > > include/libcamera/internal/matrix.h | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h > > index 0f72263f2536..4900dcc0a419 100644 > > --- a/include/libcamera/internal/matrix.h > > +++ b/include/libcamera/internal/matrix.h > > @@ -117,9 +117,11 @@ public: > > > > private: > > /* > > - * \todo The initializer is only necessary for the constructor to be > > - * constexpr in C++17. Remove the initializer as soon as we are on > > - * C++20. > > + * \todo The initializer should be only necessary for the constructor to > > + * be constexpr in C++17. However, older gcc versions (at least 9 and > > + * 12, but apparently not 10), as well as clang versions (at least 14 > > + * and 19), throw compilation errors, even with C++20. Remove the > > + * initializer once we can drop support of those old compiler versions. > > */ > > std::array<T, Rows * Cols> data_ = {}; > > }; > > > > base-commit: 5701eb5f47cf3649e2746a2082c7bb3e0a0577e8
2026. 06. 15. 19:29 keltezéssel, Laurent Pinchart írta: > On Mon, Jun 15, 2026 at 03:52:47PM +0200, Barnabás Pőcze wrote: >> 2026. 06. 09. 17:50 keltezéssel, Laurent Pinchart írta: >>> It was envisioned that the Matrix::data_ initializer could be dropped >>> once moving to C++20. Unfortunately older compilers don't agree. Update >>> the comment accordingly. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >> >> The `Vector` and `Matrix` classes should be the same in this respect, > > Agreed. > >> but they are not. My suggestion is the following: >> >> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h >> index 0f72263f25..f0d726bc75 100644 >> --- a/include/libcamera/internal/matrix.h >> +++ b/include/libcamera/internal/matrix.h >> @@ -32,9 +32,7 @@ class Matrix >> static_assert(std::is_arithmetic_v<T>, "Matrix type must be arithmetic"); >> >> public: >> - constexpr Matrix() >> - { >> - } >> + Matrix() = default; > > Could you explain why you think we should drop constexpr for the default > constructor ? Not that I object, but I'd like to record the correct > rationale in the commit message. If the defaulted constructor satisfies the requirements, then it will be constexpr implicitly. So I usually just do that, and let the compiler figure out the rest. But I think `constexpr Matrix() = default` should work as well. > >> >> Matrix(const std::array<T, Rows * Cols> &data) >> { >> @@ -48,7 +46,7 @@ public: >> >> static constexpr Matrix identity() >> { >> - Matrix ret; >> + Matrix ret = {}; >> for (size_t i = 0; i < std::min(Rows, Cols); i++) >> ret[i][i] = static_cast<T>(1); >> return ret; >> @@ -116,12 +114,7 @@ public: >> } >> >> private: >> - /* >> - * \todo The initializer is only necessary for the constructor to be >> - * constexpr in C++17. Remove the initializer as soon as we are on >> - * C++20. >> - */ >> - std::array<T, Rows * Cols> data_ = {}; >> + std::array<T, Rows * Cols> data_; >> }; >> >> #ifndef __DOXYGEN__ >> >> >> The above change should pass the CI tests with a small change to `test/matrix.cpp`: >> >> Matrix<double, 3, 3> m1 = {}; >> >> as the test assumes the matrix is zero initialized. > > That passes all CI tests, as well as my local build tests. > >> Or maybe we should just not allow the creation of uninitialized Vector/Matrix >> in the first place. > > For small matrices it probebly won't make much difference. For larger > ones, skipping initialization can improve efficiency, at the cost of > possible usage of unused values of course. Yes, but I'm wondering where can you really use a matrix with uninitialized values in it? > >>> include/libcamera/internal/matrix.h | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h >>> index 0f72263f2536..4900dcc0a419 100644 >>> --- a/include/libcamera/internal/matrix.h >>> +++ b/include/libcamera/internal/matrix.h >>> @@ -117,9 +117,11 @@ public: >>> >>> private: >>> /* >>> - * \todo The initializer is only necessary for the constructor to be >>> - * constexpr in C++17. Remove the initializer as soon as we are on >>> - * C++20. >>> + * \todo The initializer should be only necessary for the constructor to >>> + * be constexpr in C++17. However, older gcc versions (at least 9 and >>> + * 12, but apparently not 10), as well as clang versions (at least 14 >>> + * and 19), throw compilation errors, even with C++20. Remove the >>> + * initializer once we can drop support of those old compiler versions. >>> */ >>> std::array<T, Rows * Cols> data_ = {}; >>> }; >>> >>> base-commit: 5701eb5f47cf3649e2746a2082c7bb3e0a0577e8 >
diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h index 0f72263f2536..4900dcc0a419 100644 --- a/include/libcamera/internal/matrix.h +++ b/include/libcamera/internal/matrix.h @@ -117,9 +117,11 @@ public: private: /* - * \todo The initializer is only necessary for the constructor to be - * constexpr in C++17. Remove the initializer as soon as we are on - * C++20. + * \todo The initializer should be only necessary for the constructor to + * be constexpr in C++17. However, older gcc versions (at least 9 and + * 12, but apparently not 10), as well as clang versions (at least 14 + * and 19), throw compilation errors, even with C++20. Remove the + * initializer once we can drop support of those old compiler versions. */ std::array<T, Rows * Cols> data_ = {}; };
It was envisioned that the Matrix::data_ initializer could be dropped once moving to C++20. Unfortunately older compilers don't agree. Update the comment accordingly. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/matrix.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) base-commit: 5701eb5f47cf3649e2746a2082c7bb3e0a0577e8