Message ID | 20250417113539.1137936-2-barnabas.pocze@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Quoting Barnabás Pőcze (2025-04-17 12:35:38) > Implement a move constructor and move assignment operator for `ControlValue`. > The `ControlValue` type already has an "empty" state that it used when > creating a default constructed `ControlValue`, so have the moved-from > instance return to that state after move construction/assignment. > > This is useful, for example, for `std::vector` as most implementations will > use the copy constructor when reallocating if no nothrow move constructor > is available. Having a nothrow move constructor avoids the extra copies. > It is also useful when using temporaries of `ControlValue` with other > containers such as `std::optional`, and it also makes `ControlInfo` > "cheaply" move constructible/assignable. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++ > src/libcamera/controls.cpp | 17 +++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 1dc6ccffa..86245e7a9 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -14,6 +14,7 @@ > #include <stdint.h> > #include <string> > #include <unordered_map> > +#include <utility> > #include <vector> > > #include <libcamera/base/class.h> > @@ -165,6 +166,33 @@ public: > ControlValue(const ControlValue &other); > ControlValue &operator=(const ControlValue &other); > > + ControlValue(ControlValue &&other) noexcept > + : type_(other.type_), > + isArray_(std::exchange(other.isArray_, false)), > + numElements_(other.numElements_), > + storage_(std::exchange(other.storage_, {})) > + { > + other.type_ = ControlTypeNone; > + other.numElements_ = 0; > + } > + > + ControlValue &operator=(ControlValue &&other) noexcept > + { > + if (this != &other) { > + release(); > + If we invert this we can lower the indent: if (this == &other) return *this; release(); type_ = other.type_; ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + type_ = other.type_; > + isArray_ = std::exchange(other.isArray_, false); > + numElements_ = other.numElements_; > + storage_ = std::exchange(other.storage_, {}); > + > + other.type_ = ControlTypeNone; > + other.numElements_ = 0; > + } > + > + return *this; > + } > + > ControlType type() const { return type_; } > bool isNone() const { return type_ == ControlTypeNone; } > bool isArray() const { return isArray_; } > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index d384e1ef7..885287e71 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -155,6 +155,23 @@ ControlValue &ControlValue::operator=(const ControlValue &other) > return *this; > } > > +/** > + * \fn ControlValue::ControlValue(ControlValue &&other) noexcept > + * \brief Move constructor for ControlValue > + * \param[in] other The ControlValue object to move from > + * > + * Move constructs a ControlValue instance from \a other. After this opreation > + * \a other will be in the same state as a default constructed ControlValue instance. > + */ > + > +/** > + * \fn ControlValue &ControlValue::operator=(ControlValue &&other) noexcept > + * \brief Move assignment operator for ControlValue > + * \param[in] other The ControlValue object to move from > + * > + * \sa ControlValue::ControlValue(ControlValue &&other) > + */ > + > /** > * \fn ControlValue::type() > * \brief Retrieve the data type of the value > -- > 2.49.0 >
Hi Barnabás On Thu, Apr 17, 2025 at 01:35:38PM +0200, Barnabás Pőcze wrote: > Implement a move constructor and move assignment operator for `ControlValue`. > The `ControlValue` type already has an "empty" state that it used when s/it used/is used/ > creating a default constructed `ControlValue`, so have the moved-from > instance return to that state after move construction/assignment. > > This is useful, for example, for `std::vector` as most implementations will > use the copy constructor when reallocating if no nothrow move constructor > is available. Having a nothrow move constructor avoids the extra copies. > It is also useful when using temporaries of `ControlValue` with other > containers such as `std::optional`, and it also makes `ControlInfo` > "cheaply" move constructible/assignable. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++ > src/libcamera/controls.cpp | 17 +++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 1dc6ccffa..86245e7a9 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -14,6 +14,7 @@ > #include <stdint.h> > #include <string> > #include <unordered_map> > +#include <utility> > #include <vector> > > #include <libcamera/base/class.h> > @@ -165,6 +166,33 @@ public: > ControlValue(const ControlValue &other); > ControlValue &operator=(const ControlValue &other); > > + ControlValue(ControlValue &&other) noexcept > + : type_(other.type_), > + isArray_(std::exchange(other.isArray_, false)), > + numElements_(other.numElements_), > + storage_(std::exchange(other.storage_, {})) So here, compared to the copy connstructor that does a memcpy on the (now renamed) storage_.external, while here we're only copying the pointer to the allocated memory, right ? Seems sane, I wonder if there's a reason why this hadn't been done already. > + { > + other.type_ = ControlTypeNone; > + other.numElements_ = 0; I presume there's a reason why you haven't defined this using the newly introduced operator=(&&) (the same as it is done for the copy constructor) > + } > + > + ControlValue &operator=(ControlValue &&other) noexcept > + { > + if (this != &other) { > + release(); > + > + type_ = other.type_; > + isArray_ = std::exchange(other.isArray_, false); > + numElements_ = other.numElements_; > + storage_ = std::exchange(other.storage_, {}); > + > + other.type_ = ControlTypeNone; > + other.numElements_ = 0; > + } > + > + return *this; > + } > + > ControlType type() const { return type_; } > bool isNone() const { return type_ == ControlTypeNone; } > bool isArray() const { return isArray_; } > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index d384e1ef7..885287e71 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -155,6 +155,23 @@ ControlValue &ControlValue::operator=(const ControlValue &other) > return *this; > } > > +/** > + * \fn ControlValue::ControlValue(ControlValue &&other) noexcept > + * \brief Move constructor for ControlValue > + * \param[in] other The ControlValue object to move from > + * > + * Move constructs a ControlValue instance from \a other. After this opreation s/opreation/operation > + * \a other will be in the same state as a default constructed ControlValue instance. Can easily fit in 80 cols With minors fixed, and unless there are reasons I'm missing why we didn't add it some time ago: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + */ > + > +/** > + * \fn ControlValue &ControlValue::operator=(ControlValue &&other) noexcept > + * \brief Move assignment operator for ControlValue > + * \param[in] other The ControlValue object to move from > + * > + * \sa ControlValue::ControlValue(ControlValue &&other) > + */ > + > /** > * \fn ControlValue::type() > * \brief Retrieve the data type of the value > -- > 2.49.0 >
Hi 2025. 04. 18. 12:33 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Thu, Apr 17, 2025 at 01:35:38PM +0200, Barnabás Pőcze wrote: >> Implement a move constructor and move assignment operator for `ControlValue`. >> The `ControlValue` type already has an "empty" state that it used when > > s/it used/is used/ > >> creating a default constructed `ControlValue`, so have the moved-from >> instance return to that state after move construction/assignment. >> >> This is useful, for example, for `std::vector` as most implementations will >> use the copy constructor when reallocating if no nothrow move constructor >> is available. Having a nothrow move constructor avoids the extra copies. >> It is also useful when using temporaries of `ControlValue` with other >> containers such as `std::optional`, and it also makes `ControlInfo` >> "cheaply" move constructible/assignable. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++ >> src/libcamera/controls.cpp | 17 +++++++++++++++++ >> 2 files changed, 45 insertions(+) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 1dc6ccffa..86245e7a9 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -14,6 +14,7 @@ >> #include <stdint.h> >> #include <string> >> #include <unordered_map> >> +#include <utility> >> #include <vector> >> >> #include <libcamera/base/class.h> >> @@ -165,6 +166,33 @@ public: >> ControlValue(const ControlValue &other); >> ControlValue &operator=(const ControlValue &other); >> >> + ControlValue(ControlValue &&other) noexcept >> + : type_(other.type_), >> + isArray_(std::exchange(other.isArray_, false)), >> + numElements_(other.numElements_), >> + storage_(std::exchange(other.storage_, {})) > > So here, compared to the copy connstructor that does a memcpy on the > (now renamed) storage_.external, while here we're only copying the > pointer to the allocated memory, right ? Yes, the pointer (or inline value) is copied (the ownership of the allocated memory is transferred to the `this` instance), and the other is left in an "empty" state. > > Seems sane, I wonder if there's a reason why this hadn't been done > already. > >> + { >> + other.type_ = ControlTypeNone; >> + other.numElements_ = 0; > > I presume there's a reason why you haven't defined this using the > newly introduced operator=(&&) (the same as it is done for the copy > constructor) That can lead to suboptimal code in my experience (especially here since `release()` cannot be inlined), if we wanted to avoid the repetition, then I would replace `operator=` with the following: ControlValue(std::move(other)).swap(*this); return *this; Regards, Barnabás Pőcze > >> + } >> + >> + ControlValue &operator=(ControlValue &&other) noexcept >> + { >> + if (this != &other) { >> + release(); >> + >> + type_ = other.type_; >> + isArray_ = std::exchange(other.isArray_, false); >> + numElements_ = other.numElements_; >> + storage_ = std::exchange(other.storage_, {}); >> + >> + other.type_ = ControlTypeNone; >> + other.numElements_ = 0; >> + } >> + >> + return *this; >> + } >> + >> ControlType type() const { return type_; } >> bool isNone() const { return type_ == ControlTypeNone; } >> bool isArray() const { return isArray_; } >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index d384e1ef7..885287e71 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -155,6 +155,23 @@ ControlValue &ControlValue::operator=(const ControlValue &other) >> return *this; >> } >> >> +/** >> + * \fn ControlValue::ControlValue(ControlValue &&other) noexcept >> + * \brief Move constructor for ControlValue >> + * \param[in] other The ControlValue object to move from >> + * >> + * Move constructs a ControlValue instance from \a other. After this opreation > > s/opreation/operation > >> + * \a other will be in the same state as a default constructed ControlValue instance. > > Can easily fit in 80 cols > > With minors fixed, and unless there are reasons I'm missing why we > didn't add it some time ago: > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> + */ >> + >> +/** >> + * \fn ControlValue &ControlValue::operator=(ControlValue &&other) noexcept >> + * \brief Move assignment operator for ControlValue >> + * \param[in] other The ControlValue object to move from >> + * >> + * \sa ControlValue::ControlValue(ControlValue &&other) >> + */ >> + >> /** >> * \fn ControlValue::type() >> * \brief Retrieve the data type of the value >> -- >> 2.49.0 >>
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 1dc6ccffa..86245e7a9 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -14,6 +14,7 @@ #include <stdint.h> #include <string> #include <unordered_map> +#include <utility> #include <vector> #include <libcamera/base/class.h> @@ -165,6 +166,33 @@ public: ControlValue(const ControlValue &other); ControlValue &operator=(const ControlValue &other); + ControlValue(ControlValue &&other) noexcept + : type_(other.type_), + isArray_(std::exchange(other.isArray_, false)), + numElements_(other.numElements_), + storage_(std::exchange(other.storage_, {})) + { + other.type_ = ControlTypeNone; + other.numElements_ = 0; + } + + ControlValue &operator=(ControlValue &&other) noexcept + { + if (this != &other) { + release(); + + type_ = other.type_; + isArray_ = std::exchange(other.isArray_, false); + numElements_ = other.numElements_; + storage_ = std::exchange(other.storage_, {}); + + other.type_ = ControlTypeNone; + other.numElements_ = 0; + } + + return *this; + } + ControlType type() const { return type_; } bool isNone() const { return type_ == ControlTypeNone; } bool isArray() const { return isArray_; } diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index d384e1ef7..885287e71 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -155,6 +155,23 @@ ControlValue &ControlValue::operator=(const ControlValue &other) return *this; } +/** + * \fn ControlValue::ControlValue(ControlValue &&other) noexcept + * \brief Move constructor for ControlValue + * \param[in] other The ControlValue object to move from + * + * Move constructs a ControlValue instance from \a other. After this opreation + * \a other will be in the same state as a default constructed ControlValue instance. + */ + +/** + * \fn ControlValue &ControlValue::operator=(ControlValue &&other) noexcept + * \brief Move assignment operator for ControlValue + * \param[in] other The ControlValue object to move from + * + * \sa ControlValue::ControlValue(ControlValue &&other) + */ + /** * \fn ControlValue::type() * \brief Retrieve the data type of the value
Implement a move constructor and move assignment operator for `ControlValue`. The `ControlValue` type already has an "empty" state that it used when creating a default constructed `ControlValue`, so have the moved-from instance return to that state after move construction/assignment. This is useful, for example, for `std::vector` as most implementations will use the copy constructor when reallocating if no nothrow move constructor is available. Having a nothrow move constructor avoids the extra copies. It is also useful when using temporaries of `ControlValue` with other containers such as `std::optional`, and it also makes `ControlInfo` "cheaply" move constructible/assignable. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++ src/libcamera/controls.cpp | 17 +++++++++++++++++ 2 files changed, 45 insertions(+)