[v1,2/3] libcamera: controls: Implement move ctor/assignment
diff mbox series

Message ID 20250417113539.1137936-2-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1,1/3] libcamera: controls: Give name to the union containing storage
Related show

Commit Message

Barnabás Pőcze April 17, 2025, 11:35 a.m. UTC
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(+)

Comments

Kieran Bingham April 17, 2025, 3:29 p.m. UTC | #1
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
>
Jacopo Mondi April 18, 2025, 10:33 a.m. UTC | #2
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
>
Barnabás Pőcze April 18, 2025, 12:18 p.m. UTC | #3
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
>>

Patch
diff mbox series

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