Message ID | 20250417113539.1137936-3-barnabas.pocze@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Quoting Barnabás Pőcze (2025-04-17 12:35:39) > Implement both free and member function `swap()` for `ControlValue`. > The general `std::swap()` swaps two values by combining a move construction > and two move assignments, but for `ControlValue` a simpler implementation > can be provided by just swapping the members. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/controls.h | 24 ++++++++++++++++++++++++ > src/libcamera/controls.cpp | 15 +++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 86245e7a9..27b314dbc 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -261,6 +261,30 @@ public: > void reserve(ControlType type, bool isArray = false, > std::size_t numElements = 1); > > + void swap(ControlValue &other) noexcept > + { > + { > + auto tmp = type_; > + type_ = other.type_; > + other.type_ = tmp; > + } I fear I'm missing something obvious ... why are type_ and numElements_ not items that can simply be swapped with std::swap() ? Is it because of the need to use auto on the type? or something else? Should this implementation go into controls.cpp ? or is it required/better inline in the header ? > + > + std::swap(isArray_, other.isArray_); > + > + { > + auto tmp = numElements_; > + numElements_ = other.numElements_; > + other.numElements_ = tmp; > + } > + > + std::swap(storage_, other.storage_); > + } > + > + friend void swap(ControlValue &a, ControlValue &b) noexcept > + { > + a.swap(b); > + } > + > private: > ControlType type_ : 8; > bool isArray_; > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 885287e71..c1ff60f39 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -411,6 +411,21 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > storage_.external = new uint8_t[newSize]; > } > > +/** > + * \fn ControlValue::swap(ControlValue &other) noexcept > + * \brief Swap two control values > + * > + * This function swaps the contained value of \a this with that of \a other. > + */ > + > +/** > + * \fn ControlValue::swap(ControlValue &a, ControlValue &b) noexcept > + * \brief Swap two control values > + * > + * This function swaps the contained value of \a a with that of \a b. > + * \sa ControlValue::swap() > + */ > + > /** > * \class ControlId > * \brief Control static metadata > -- > 2.49.0 >
Hi 2025. 04. 17. 17:34 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-04-17 12:35:39) >> Implement both free and member function `swap()` for `ControlValue`. >> The general `std::swap()` swaps two values by combining a move construction >> and two move assignments, but for `ControlValue` a simpler implementation >> can be provided by just swapping the members. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/controls.h | 24 ++++++++++++++++++++++++ >> src/libcamera/controls.cpp | 15 +++++++++++++++ >> 2 files changed, 39 insertions(+) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 86245e7a9..27b314dbc 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -261,6 +261,30 @@ public: >> void reserve(ControlType type, bool isArray = false, >> std::size_t numElements = 1); >> >> + void swap(ControlValue &other) noexcept >> + { >> + { >> + auto tmp = type_; >> + type_ = other.type_; >> + other.type_ = tmp; >> + } > > I fear I'm missing something obvious ... why are type_ and numElements_ > not items that can simply be swapped with std::swap() ? > > Is it because of the need to use auto on the type? or something else? They are bit fields, references to bit fields cannot be taken, so `std::swap()` cannot be used. > > Should this implementation go into controls.cpp ? or is it > required/better inline in the header ? I would leave it in the header file, it's not too many instructions. Although... it is not optimal in my opinion. I think the compilers don't want to merge the loads/stores, possibly because of the padding. Regards, Barnabás Pőcze > >> + >> + std::swap(isArray_, other.isArray_); >> + >> + { >> + auto tmp = numElements_; >> + numElements_ = other.numElements_; >> + other.numElements_ = tmp; >> + } >> + >> + std::swap(storage_, other.storage_); >> + } >> + >> + friend void swap(ControlValue &a, ControlValue &b) noexcept >> + { >> + a.swap(b); >> + } >> + >> private: >> ControlType type_ : 8; >> bool isArray_; >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index 885287e71..c1ff60f39 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -411,6 +411,21 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen >> storage_.external = new uint8_t[newSize]; >> } >> >> +/** >> + * \fn ControlValue::swap(ControlValue &other) noexcept >> + * \brief Swap two control values >> + * >> + * This function swaps the contained value of \a this with that of \a other. >> + */ >> + >> +/** >> + * \fn ControlValue::swap(ControlValue &a, ControlValue &b) noexcept >> + * \brief Swap two control values >> + * >> + * This function swaps the contained value of \a a with that of \a b. >> + * \sa ControlValue::swap() >> + */ >> + >> /** >> * \class ControlId >> * \brief Control static metadata >> -- >> 2.49.0 >>
Quoting Barnabás Pőcze (2025-04-17 17:03:07) > Hi > > > 2025. 04. 17. 17:34 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-04-17 12:35:39) > >> Implement both free and member function `swap()` for `ControlValue`. > >> The general `std::swap()` swaps two values by combining a move construction > >> and two move assignments, but for `ControlValue` a simpler implementation > >> can be provided by just swapping the members. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> include/libcamera/controls.h | 24 ++++++++++++++++++++++++ > >> src/libcamera/controls.cpp | 15 +++++++++++++++ > >> 2 files changed, 39 insertions(+) > >> > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >> index 86245e7a9..27b314dbc 100644 > >> --- a/include/libcamera/controls.h > >> +++ b/include/libcamera/controls.h > >> @@ -261,6 +261,30 @@ public: > >> void reserve(ControlType type, bool isArray = false, > >> std::size_t numElements = 1); > >> > >> + void swap(ControlValue &other) noexcept > >> + { > >> + { > >> + auto tmp = type_; > >> + type_ = other.type_; > >> + other.type_ = tmp; > >> + } > > > > I fear I'm missing something obvious ... why are type_ and numElements_ > > not items that can simply be swapped with std::swap() ? > > > > Is it because of the need to use auto on the type? or something else? > > They are bit fields, references to bit fields cannot be taken, so > `std::swap()` cannot be used. > > > > > Should this implementation go into controls.cpp ? or is it > > required/better inline in the header ? > > I would leave it in the header file, it's not too many instructions. > > Although... it is not optimal in my opinion. I think the compilers > don't want to merge the loads/stores, possibly because of the padding. > This won't be on a performance hotpath though will it ? So I don't think a couple of extra operations here will be too upsetting. Anyway, if it's clear that std::swap can't be used directly - perhaps a short oneline comment could describe that to stop someone coming in to optimise it into a swap call ... but otherwise: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Regards, > Barnabás Pőcze > > > > >> + > >> + std::swap(isArray_, other.isArray_); > >> + > >> + { > >> + auto tmp = numElements_; > >> + numElements_ = other.numElements_; > >> + other.numElements_ = tmp; > >> + } > >> + > >> + std::swap(storage_, other.storage_); > >> + } > >> + > >> + friend void swap(ControlValue &a, ControlValue &b) noexcept > >> + { > >> + a.swap(b); > >> + } > >> + > >> private: > >> ControlType type_ : 8; > >> bool isArray_; > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > >> index 885287e71..c1ff60f39 100644 > >> --- a/src/libcamera/controls.cpp > >> +++ b/src/libcamera/controls.cpp > >> @@ -411,6 +411,21 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > >> storage_.external = new uint8_t[newSize]; > >> } > >> > >> +/** > >> + * \fn ControlValue::swap(ControlValue &other) noexcept > >> + * \brief Swap two control values > >> + * > >> + * This function swaps the contained value of \a this with that of \a other. > >> + */ > >> + > >> +/** > >> + * \fn ControlValue::swap(ControlValue &a, ControlValue &b) noexcept > >> + * \brief Swap two control values > >> + * > >> + * This function swaps the contained value of \a a with that of \a b. > >> + * \sa ControlValue::swap() > >> + */ > >> + > >> /** > >> * \class ControlId > >> * \brief Control static metadata > >> -- > >> 2.49.0 > >> >
On Fri, Apr 18, 2025 at 01:10:06AM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2025-04-17 17:03:07) > > Hi > > > > > > 2025. 04. 17. 17:34 keltezéssel, Kieran Bingham írta: > > > Quoting Barnabás Pőcze (2025-04-17 12:35:39) > > >> Implement both free and member function `swap()` for `ControlValue`. > > >> The general `std::swap()` swaps two values by combining a move construction > > >> and two move assignments, but for `ControlValue` a simpler implementation > > >> can be provided by just swapping the members. > > >> > > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > >> --- > > >> include/libcamera/controls.h | 24 ++++++++++++++++++++++++ > > >> src/libcamera/controls.cpp | 15 +++++++++++++++ > > >> 2 files changed, 39 insertions(+) > > >> > > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > >> index 86245e7a9..27b314dbc 100644 > > >> --- a/include/libcamera/controls.h > > >> +++ b/include/libcamera/controls.h > > >> @@ -261,6 +261,30 @@ public: > > >> void reserve(ControlType type, bool isArray = false, > > >> std::size_t numElements = 1); > > >> > > >> + void swap(ControlValue &other) noexcept > > >> + { > > >> + { > > >> + auto tmp = type_; > > >> + type_ = other.type_; > > >> + other.type_ = tmp; > > >> + } > > > > > > I fear I'm missing something obvious ... why are type_ and numElements_ > > > not items that can simply be swapped with std::swap() ? > > > > > > Is it because of the need to use auto on the type? or something else? > > > > They are bit fields, references to bit fields cannot be taken, so > > `std::swap()` cannot be used. > > > > > > > > Should this implementation go into controls.cpp ? or is it > > > required/better inline in the header ? > > > > I would leave it in the header file, it's not too many instructions. > > > > Although... it is not optimal in my opinion. I think the compilers > > don't want to merge the loads/stores, possibly because of the padding. > > > > This won't be on a performance hotpath though will it ? So I don't think > a couple of extra operations here will be too upsetting. > > Anyway, if it's clear that std::swap can't be used directly - perhaps a > short oneline comment could describe that to stop someone coming in to > optimise it into a swap call ... but otherwise: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > With the above suggestions Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > > > Regards, > > Barnabás Pőcze > > > > > > > >> + > > >> + std::swap(isArray_, other.isArray_); > > >> + > > >> + { > > >> + auto tmp = numElements_; > > >> + numElements_ = other.numElements_; > > >> + other.numElements_ = tmp; > > >> + } > > >> + > > >> + std::swap(storage_, other.storage_); > > >> + } > > >> + > > >> + friend void swap(ControlValue &a, ControlValue &b) noexcept > > >> + { > > >> + a.swap(b); > > >> + } > > >> + > > >> private: > > >> ControlType type_ : 8; > > >> bool isArray_; > > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > >> index 885287e71..c1ff60f39 100644 > > >> --- a/src/libcamera/controls.cpp > > >> +++ b/src/libcamera/controls.cpp > > >> @@ -411,6 +411,21 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > > >> storage_.external = new uint8_t[newSize]; > > >> } > > >> > > >> +/** > > >> + * \fn ControlValue::swap(ControlValue &other) noexcept > > >> + * \brief Swap two control values > > >> + * > > >> + * This function swaps the contained value of \a this with that of \a other. > > >> + */ > > >> + > > >> +/** > > >> + * \fn ControlValue::swap(ControlValue &a, ControlValue &b) noexcept > > >> + * \brief Swap two control values > > >> + * > > >> + * This function swaps the contained value of \a a with that of \a b. > > >> + * \sa ControlValue::swap() > > >> + */ > > >> + > > >> /** > > >> * \class ControlId > > >> * \brief Control static metadata > > >> -- > > >> 2.49.0 > > >> > >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 86245e7a9..27b314dbc 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -261,6 +261,30 @@ public: void reserve(ControlType type, bool isArray = false, std::size_t numElements = 1); + void swap(ControlValue &other) noexcept + { + { + auto tmp = type_; + type_ = other.type_; + other.type_ = tmp; + } + + std::swap(isArray_, other.isArray_); + + { + auto tmp = numElements_; + numElements_ = other.numElements_; + other.numElements_ = tmp; + } + + std::swap(storage_, other.storage_); + } + + friend void swap(ControlValue &a, ControlValue &b) noexcept + { + a.swap(b); + } + private: ControlType type_ : 8; bool isArray_; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 885287e71..c1ff60f39 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -411,6 +411,21 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen storage_.external = new uint8_t[newSize]; } +/** + * \fn ControlValue::swap(ControlValue &other) noexcept + * \brief Swap two control values + * + * This function swaps the contained value of \a this with that of \a other. + */ + +/** + * \fn ControlValue::swap(ControlValue &a, ControlValue &b) noexcept + * \brief Swap two control values + * + * This function swaps the contained value of \a a with that of \a b. + * \sa ControlValue::swap() + */ + /** * \class ControlId * \brief Control static metadata
Implement both free and member function `swap()` for `ControlValue`. The general `std::swap()` swaps two values by combining a move construction and two move assignments, but for `ControlValue` a simpler implementation can be provided by just swapping the members. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/controls.h | 24 ++++++++++++++++++++++++ src/libcamera/controls.cpp | 15 +++++++++++++++ 2 files changed, 39 insertions(+)