[v2,3/5] libcamera: controls: Implement move ctor/assignment
diff mbox series

Message ID 20250421153556.171192-4-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: controls: Move constructor/assignment + swap
Related show

Commit Message

Barnabás Pőcze April 21, 2025, 3:35 p.m. UTC 
Implement a move constructor and move assignment operator for `ControlValue`.
The `ControlValue` type already has an "empty" state that is 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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
 src/libcamera/controls.cpp   | 18 ++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Jacopo Mondi Nov. 21, 2025, 11:47 a.m. UTC | #1 
Hi Barnabás

On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
>  src/libcamera/controls.cpp   | 18 ++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 3f3580036..866d667c4 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,31 @@ public:
>  	ControlValue(const ControlValue &other);
>  	ControlValue &operator=(const ControlValue &other);
>
> +	ControlValue(ControlValue &&other) noexcept
> +		: type_(other.type_),

Is it necessary to reset the other's type to ControlTypeNone ?

> +		  isArray_(std::exchange(other.isArray_, false)),
> +		  numElements_(std::exchange(other.numElements_, 0)),
> +		  storage_(std::exchange(other.storage_, {}))
> +	{
> +		other.type_ = ControlTypeNone;
> +	}
> +
> +	ControlValue &operator=(ControlValue &&other) noexcept
> +	{
> +		if (this != &other) {
> +			release();
> +
> +			type_ = other.type_;
> +			isArray_ = std::exchange(other.isArray_, false);
> +			numElements_ = std::exchange(other.numElements_, 0);
> +			storage_ = std::exchange(other.storage_, {});
> +
> +			other.type_ = ControlTypeNone;
> +		}
> +
> +		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..135e87613 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -155,6 +155,24 @@ 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 operation \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
>
Barnabás Pőcze Nov. 21, 2025, 12:49 p.m. UTC | #2 
2025. 11. 21. 12:47 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
>>   src/libcamera/controls.cpp   | 18 ++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 3f3580036..866d667c4 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,31 @@ public:
>>   	ControlValue(const ControlValue &other);
>>   	ControlValue &operator=(const ControlValue &other);
>>
>> +	ControlValue(ControlValue &&other) noexcept
>> +		: type_(other.type_),
> 
> Is it necessary to reset the other's type to ControlTypeNone ?

I think so. Since the underlying storage is moved, the previous type cannot always
be kept because there would no longer be storage (e.g. if it is a bigger array).
So setting it to none unconditionally seems the best decision to me.


> 
>> +		  isArray_(std::exchange(other.isArray_, false)),
>> +		  numElements_(std::exchange(other.numElements_, 0)),
>> +		  storage_(std::exchange(other.storage_, {}))
>> +	{
>> +		other.type_ = ControlTypeNone;
>> +	}
>> +
>> +	ControlValue &operator=(ControlValue &&other) noexcept
>> +	{
>> +		if (this != &other) {
>> +			release();
>> +
>> +			type_ = other.type_;
>> +			isArray_ = std::exchange(other.isArray_, false);
>> +			numElements_ = std::exchange(other.numElements_, 0);
>> +			storage_ = std::exchange(other.storage_, {});
>> +
>> +			other.type_ = ControlTypeNone;
>> +		}
>> +
>> +		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..135e87613 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -155,6 +155,24 @@ 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 operation \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
>>
Laurent Pinchart Sept. 1, 2026, 1:37 p.m. UTC | #3 
Hi Barnabás,

Thank you for the patch.

On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
>  src/libcamera/controls.cpp   | 18 ++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 3f3580036..866d667c4 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,31 @@ public:
>  	ControlValue(const ControlValue &other);
>  	ControlValue &operator=(const ControlValue &other);
>  
> +	ControlValue(ControlValue &&other) noexcept
> +		: type_(other.type_),
> +		  isArray_(std::exchange(other.isArray_, false)),
> +		  numElements_(std::exchange(other.numElements_, 0)),
> +		  storage_(std::exchange(other.storage_, {}))
> +	{
> +		other.type_ = ControlTypeNone;
> +	}
> +
> +	ControlValue &operator=(ControlValue &&other) noexcept
> +	{
> +		if (this != &other) {
> +			release();
> +
> +			type_ = other.type_;
> +			isArray_ = std::exchange(other.isArray_, false);
> +			numElements_ = std::exchange(other.numElements_, 0);
> +			storage_ = std::exchange(other.storage_, {});
> +
> +			other.type_ = ControlTypeNone;
> +		}
> +
> +		return *this;
> +	}

The copy constructor and assignement operator are not inline. Is there a
reason to inline the move versions ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	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..135e87613 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -155,6 +155,24 @@ 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 operation \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
Barnabás Pőcze Sept. 2, 2026, 12:41 p.m. UTC | #4 
2026. 09. 01. 15:37 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
>>   src/libcamera/controls.cpp   | 18 ++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 3f3580036..866d667c4 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,31 @@ public:
>>   	ControlValue(const ControlValue &other);
>>   	ControlValue &operator=(const ControlValue &other);
>>   
>> +	ControlValue(ControlValue &&other) noexcept
>> +		: type_(other.type_),
>> +		  isArray_(std::exchange(other.isArray_, false)),
>> +		  numElements_(std::exchange(other.numElements_, 0)),
>> +		  storage_(std::exchange(other.storage_, {}))
>> +	{
>> +		other.type_ = ControlTypeNone;
>> +	}
>> +
>> +	ControlValue &operator=(ControlValue &&other) noexcept
>> +	{
>> +		if (this != &other) {
>> +			release();
>> +
>> +			type_ = other.type_;
>> +			isArray_ = std::exchange(other.isArray_, false);
>> +			numElements_ = std::exchange(other.numElements_, 0);
>> +			storage_ = std::exchange(other.storage_, {});
>> +
>> +			other.type_ = ControlTypeNone;
>> +		}
>> +
>> +		return *this;
>> +	}
> 
> The copy constructor and assignement operator are not inline. Is there a
> reason to inline the move versions ?

I think these can be well optimized by the compiler into fairly short machine code,
so that consideration made me keep them in the header file.


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +
>>   	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..135e87613 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -155,6 +155,24 @@ 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 operation \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
>
Laurent Pinchart Sept. 2, 2026, 1:24 p.m. UTC | #5 
On Wed, Sep 02, 2026 at 02:41:35PM +0200, Barnabás Pőcze wrote:
> 2026. 09. 01. 15:37 keltezéssel, Laurent Pinchart írta:
> > On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> ---
> >>   include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
> >>   src/libcamera/controls.cpp   | 18 ++++++++++++++++++
> >>   2 files changed, 44 insertions(+)
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> index 3f3580036..866d667c4 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,31 @@ public:
> >>   	ControlValue(const ControlValue &other);
> >>   	ControlValue &operator=(const ControlValue &other);
> >>   
> >> +	ControlValue(ControlValue &&other) noexcept
> >> +		: type_(other.type_),
> >> +		  isArray_(std::exchange(other.isArray_, false)),
> >> +		  numElements_(std::exchange(other.numElements_, 0)),
> >> +		  storage_(std::exchange(other.storage_, {}))
> >> +	{
> >> +		other.type_ = ControlTypeNone;
> >> +	}
> >> +
> >> +	ControlValue &operator=(ControlValue &&other) noexcept
> >> +	{
> >> +		if (this != &other) {
> >> +			release();
> >> +
> >> +			type_ = other.type_;
> >> +			isArray_ = std::exchange(other.isArray_, false);
> >> +			numElements_ = std::exchange(other.numElements_, 0);
> >> +			storage_ = std::exchange(other.storage_, {});
> >> +
> >> +			other.type_ = ControlTypeNone;
> >> +		}
> >> +
> >> +		return *this;
> >> +	}
> > 
> > The copy constructor and assignement operator are not inline. Is there a
> > reason to inline the move versions ?
> 
> I think these can be well optimized by the compiler into fairly short machine code,
> so that consideration made me keep them in the header file.

Should we inline the copy versions too then ? That can be done
separately.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> +
> >>   	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..135e87613 100644
> >> --- a/src/libcamera/controls.cpp
> >> +++ b/src/libcamera/controls.cpp
> >> @@ -155,6 +155,24 @@ 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 operation \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
Barnabás Pőcze Sept. 3, 2026, 8:35 a.m. UTC | #6 
2026. 09. 02. 15:24 keltezéssel, Laurent Pinchart írta:
> On Wed, Sep 02, 2026 at 02:41:35PM +0200, Barnabás Pőcze wrote:
>> 2026. 09. 01. 15:37 keltezéssel, Laurent Pinchart írta:
>>> On Mon, Apr 21, 2025 at 05:35:54PM +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 is 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>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>> ---
>>>>    include/libcamera/controls.h | 26 ++++++++++++++++++++++++++
>>>>    src/libcamera/controls.cpp   | 18 ++++++++++++++++++
>>>>    2 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>> index 3f3580036..866d667c4 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,31 @@ public:
>>>>    	ControlValue(const ControlValue &other);
>>>>    	ControlValue &operator=(const ControlValue &other);
>>>>    
>>>> +	ControlValue(ControlValue &&other) noexcept
>>>> +		: type_(other.type_),
>>>> +		  isArray_(std::exchange(other.isArray_, false)),
>>>> +		  numElements_(std::exchange(other.numElements_, 0)),
>>>> +		  storage_(std::exchange(other.storage_, {}))
>>>> +	{
>>>> +		other.type_ = ControlTypeNone;
>>>> +	}
>>>> +
>>>> +	ControlValue &operator=(ControlValue &&other) noexcept
>>>> +	{
>>>> +		if (this != &other) {
>>>> +			release();
>>>> +
>>>> +			type_ = other.type_;
>>>> +			isArray_ = std::exchange(other.isArray_, false);
>>>> +			numElements_ = std::exchange(other.numElements_, 0);
>>>> +			storage_ = std::exchange(other.storage_, {});
>>>> +
>>>> +			other.type_ = ControlTypeNone;
>>>> +		}
>>>> +
>>>> +		return *this;
>>>> +	}
>>>
>>> The copy constructor and assignement operator are not inline. Is there a
>>> reason to inline the move versions ?
>>
>> I think these can be well optimized by the compiler into fairly short machine code,
>> so that consideration made me keep them in the header file.
> 
> Should we inline the copy versions too then ? That can be done
> separately.

They essentially just call `set()`, so possibly a worthwhile change.


> 
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>> +
>>>>    	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..135e87613 100644
>>>> --- a/src/libcamera/controls.cpp
>>>> +++ b/src/libcamera/controls.cpp
>>>> @@ -155,6 +155,24 @@ 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 operation \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
>

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 3f3580036..866d667c4 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,31 @@  public:
 	ControlValue(const ControlValue &other);
 	ControlValue &operator=(const ControlValue &other);
 
+	ControlValue(ControlValue &&other) noexcept
+		: type_(other.type_),
+		  isArray_(std::exchange(other.isArray_, false)),
+		  numElements_(std::exchange(other.numElements_, 0)),
+		  storage_(std::exchange(other.storage_, {}))
+	{
+		other.type_ = ControlTypeNone;
+	}
+
+	ControlValue &operator=(ControlValue &&other) noexcept
+	{
+		if (this != &other) {
+			release();
+
+			type_ = other.type_;
+			isArray_ = std::exchange(other.isArray_, false);
+			numElements_ = std::exchange(other.numElements_, 0);
+			storage_ = std::exchange(other.storage_, {});
+
+			other.type_ = ControlTypeNone;
+		}
+
+		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..135e87613 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -155,6 +155,24 @@  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 operation \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