| Message ID | 20250310170356.185368-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Mar 10, 2025 at 06:03:56PM +0100, Barnabás Pőcze wrote: > The `dynamic_extent` specialization is currently not trivially copyable > unlike its standard counterpart, `std::span`. This is because the copy > assignment operator is user-defined. Explicitly default it just like > it is done in the main template definition. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > include/libcamera/base/span.h | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > index 92cce4f05..806db106e 100644 > --- a/include/libcamera/base/span.h > +++ b/include/libcamera/base/span.h > @@ -346,13 +346,7 @@ public: > } > > constexpr Span(const Span &other) noexcept = default; > - > - constexpr Span &operator=(const Span &other) noexcept > - { > - data_ = other.data_; > - size_ = other.size_; > - return *this; > - } > + constexpr Span &operator=(const Span &other) noexcept = default; > > constexpr iterator begin() const { return data(); } > constexpr const_iterator cbegin() const { return begin(); } > -- > 2.48.1 >
Quoting Barnabás Pőcze (2025-03-10 17:03:56) > The `dynamic_extent` specialization is currently not trivially copyable > unlike its standard counterpart, `std::span`. This is because the copy > assignment operator is user-defined. Explicitly default it just like > it is done in the main template definition. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/base/span.h | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > index 92cce4f05..806db106e 100644 > --- a/include/libcamera/base/span.h > +++ b/include/libcamera/base/span.h > @@ -346,13 +346,7 @@ public: > } > > constexpr Span(const Span &other) noexcept = default; > - > - constexpr Span &operator=(const Span &other) noexcept > - { > - data_ = other.data_; > - size_ = other.size_; > - return *this; > - } > + constexpr Span &operator=(const Span &other) noexcept = default; This sounds sane, as the default copy should do identically as above I think. But wasn't there some rule of 5+-2 that says you're supposed to define all if you define one ? Is there any ABI breakage in this patch ? Or will it compile to something directly compatible ? For the change itself: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> But if it's an ABI change I'd like to batch up all the ABI changes together. -- Kieran > > constexpr iterator begin() const { return data(); } > constexpr const_iterator cbegin() const { return begin(); } > -- > 2.48.1 >
Hi 2025. 03. 17. 22:53 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-03-10 17:03:56) >> The `dynamic_extent` specialization is currently not trivially copyable >> unlike its standard counterpart, `std::span`. This is because the copy >> assignment operator is user-defined. Explicitly default it just like >> it is done in the main template definition. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/base/span.h | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h >> index 92cce4f05..806db106e 100644 >> --- a/include/libcamera/base/span.h >> +++ b/include/libcamera/base/span.h >> @@ -346,13 +346,7 @@ public: >> } >> >> constexpr Span(const Span &other) noexcept = default; >> - >> - constexpr Span &operator=(const Span &other) noexcept >> - { >> - data_ = other.data_; >> - size_ = other.size_; >> - return *this; >> - } >> + constexpr Span &operator=(const Span &other) noexcept = default; > > This sounds sane, as the default copy should do identically as above I > think. > > But wasn't there some rule of 5+-2 that says you're supposed to define > all if you define one ? Rule of 0/3/5: https://en.cppreference.com/w/cpp/language/rule_of_three I think another option would be removing all special member functions and letting the compiler implicitly define them (rule of 0). I guess I went with this change because it was the simplest/shortest considering the current code. > > Is there any ABI breakage in this patch ? Or will it compile to > something directly compatible ? I'm pretty sure even if it is theoretically an ABI break, it will not be one in practice. Regards, Barnabás Pőcze > > For the change itself: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > But if it's an ABI change I'd like to batch up all the ABI changes > together. > > -- > Kieran > > >> >> constexpr iterator begin() const { return data(); } >> constexpr const_iterator cbegin() const { return begin(); } >> -- >> 2.48.1 >>
diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h index 92cce4f05..806db106e 100644 --- a/include/libcamera/base/span.h +++ b/include/libcamera/base/span.h @@ -346,13 +346,7 @@ public: } constexpr Span(const Span &other) noexcept = default; - - constexpr Span &operator=(const Span &other) noexcept - { - data_ = other.data_; - size_ = other.size_; - return *this; - } + constexpr Span &operator=(const Span &other) noexcept = default; constexpr iterator begin() const { return data(); } constexpr const_iterator cbegin() const { return begin(); }
The `dynamic_extent` specialization is currently not trivially copyable unlike its standard counterpart, `std::span`. This is because the copy assignment operator is user-defined. Explicitly default it just like it is done in the main template definition. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/base/span.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)