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

[Jacopo Mondi]

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

With the above suggestions
Reviewed-by: Jacopo Mondi <jacopo.mondi at 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
> > >>
> >