[PATCH v1 3/3] libcamera: controls: Implement `swap()`

[Barnabás Pőcze]

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 at 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
>>