[v1,3/3] libcamera: controls: Implement `swap()`
diff mbox series

Message ID 20250417113539.1137936-3-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 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(+)

Comments

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

Patch
diff mbox series

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