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

[Kieran Bingham]

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

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