[v2,4/5] libcamera: controls: Implement `swap()`
diff mbox series

Message ID 20250421153556.171192-5-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 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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/controls.h | 19 +++++++++++++++++++
 src/libcamera/controls.cpp   | 15 +++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Laurent Pinchart Sept. 1, 2026, 1:50 p.m. UTC | #1 
On Mon, Apr 21, 2025 at 05:35:55PM +0200, Barnabás Pőcze wrote:
> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/controls.h | 19 +++++++++++++++++++
>  src/libcamera/controls.cpp   | 15 +++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 866d667c4..62ee5ebd6 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -259,6 +259,25 @@ public:
>  	void reserve(ControlType type, bool isArray = false,
>  		     std::size_t numElements = 1);
>  
> +	void swap(ControlValue &other) noexcept
> +	{
> +		/* `type_` is a bit field, so `std::swap()` cannot be used. */

We could set the underlying type of ControlType to uint8_t. That would
allow usage of std::exchange in the previous patch and std::swap here.

> +		{
> +			auto tmp = type_;
> +			type_ = other.type_;
> +			other.type_ = tmp;
> +		}
> +
> +		std::swap(isArray_, other.isArray_);
> +		std::swap(numElements_, other.numElements_);
> +		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 135e87613..ae54f8c77 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -412,6 +412,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
Barnabás Pőcze Sept. 2, 2026, 12:43 p.m. UTC | #2 
2026. 09. 01. 15:50 keltezéssel, Laurent Pinchart írta:
> On Mon, Apr 21, 2025 at 05:35:55PM +0200, Barnabás Pőcze wrote:
>> 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   include/libcamera/controls.h | 19 +++++++++++++++++++
>>   src/libcamera/controls.cpp   | 15 +++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 866d667c4..62ee5ebd6 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -259,6 +259,25 @@ public:
>>   	void reserve(ControlType type, bool isArray = false,
>>   		     std::size_t numElements = 1);
>>   
>> +	void swap(ControlValue &other) noexcept
>> +	{
>> +		/* `type_` is a bit field, so `std::swap()` cannot be used. */
> 
> We could set the underlying type of ControlType to uint8_t. That would
> allow usage of std::exchange in the previous patch and std::swap here.

I gather you'd strongly prefer this amendment?


> 
>> +		{
>> +			auto tmp = type_;
>> +			type_ = other.type_;
>> +			other.type_ = tmp;
>> +		}
>> +
>> +		std::swap(isArray_, other.isArray_);
>> +		std::swap(numElements_, other.numElements_);
>> +		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 135e87613..ae54f8c77 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -412,6 +412,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
>
Laurent Pinchart Sept. 2, 2026, 1:22 p.m. UTC | #3 
On Wed, Sep 02, 2026 at 02:43:35PM +0200, Barnabás Pőcze wrote:
> 2026. 09. 01. 15:50 keltezéssel, Laurent Pinchart írta:
> > On Mon, Apr 21, 2025 at 05:35:55PM +0200, Barnabás Pőcze wrote:
> >> 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> ---
> >>   include/libcamera/controls.h | 19 +++++++++++++++++++
> >>   src/libcamera/controls.cpp   | 15 +++++++++++++++
> >>   2 files changed, 34 insertions(+)
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> index 866d667c4..62ee5ebd6 100644
> >> --- a/include/libcamera/controls.h
> >> +++ b/include/libcamera/controls.h
> >> @@ -259,6 +259,25 @@ public:
> >>   	void reserve(ControlType type, bool isArray = false,
> >>   		     std::size_t numElements = 1);
> >>   
> >> +	void swap(ControlValue &other) noexcept
> >> +	{
> >> +		/* `type_` is a bit field, so `std::swap()` cannot be used. */
> > 
> > We could set the underlying type of ControlType to uint8_t. That would
> > allow usage of std::exchange in the previous patch and std::swap here.
> 
> I gather you'd strongly prefer this amendment?

Not a strong one, but I think it would lead to nicer code and I don't
see a drawback.

> >> +		{
> >> +			auto tmp = type_;
> >> +			type_ = other.type_;
> >> +			other.type_ = tmp;
> >> +		}
> >> +
> >> +		std::swap(isArray_, other.isArray_);
> >> +		std::swap(numElements_, other.numElements_);
> >> +		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 135e87613..ae54f8c77 100644
> >> --- a/src/libcamera/controls.cpp
> >> +++ b/src/libcamera/controls.cpp
> >> @@ -412,6 +412,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

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 866d667c4..62ee5ebd6 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -259,6 +259,25 @@  public:
 	void reserve(ControlType type, bool isArray = false,
 		     std::size_t numElements = 1);
 
+	void swap(ControlValue &other) noexcept
+	{
+		/* `type_` is a bit field, so `std::swap()` cannot be used. */
+		{
+			auto tmp = type_;
+			type_ = other.type_;
+			other.type_ = tmp;
+		}
+
+		std::swap(isArray_, other.isArray_);
+		std::swap(numElements_, other.numElements_);
+		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 135e87613..ae54f8c77 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -412,6 +412,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