[PATCH v2 04/17] libcamera: vector: Add a Span based constructor

[Laurent Pinchart]

On Tue, Apr 01, 2025 at 04:20:15PM +0200, Stefan Klug wrote:
> On Mon, Mar 31, 2025 at 06:05:28PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-19 16:11:09)
> > > Now that Matrix has a Span based constructor, add the same for Vector.
> > 
> > Well, with that logic, someone else has a yacht - so I deserve one ...
> > :-)
> 
> Nice indeed. I'd like to have a plane :-)
> 
> > 
> > I assume Vectors deserve span based constructors because it benefits
> > them directly?
> 
> Fixed the commit message.
> 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Added this patch
> > > ---
> > >  include/libcamera/internal/vector.h | 12 ++++++++----
> > >  src/libcamera/vector.cpp            |  8 ++++++++
> > >  2 files changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h
> > > index a67a09474204..66cc5ac988c2 100644
> > > --- a/include/libcamera/internal/vector.h
> > > +++ b/include/libcamera/internal/vector.h
> > > @@ -40,10 +40,14 @@ public:
> > >                 data_.fill(scalar);
> > >         }
> > >  
> > > -       constexpr Vector(const std::array<T, Rows> &data)
> > > +       Vector(const std::array<T, Rows> &data)
> > >         {
> > > -               for (unsigned int i = 0; i < Rows; i++)
> > > -                       data_[i] = data[i];
> > > +               std::copy(data.begin(), data.end(), data_.begin());
> > > +       }
> > 
> > This loses a constexpr ... what's the downside here? At least that
> > should be highlighted/discussed in the commit message.
> 
> Actually this is a bit of a mess. I changed to std::copy to align with
> the Matrix implementation. But std::copy is not constexpr until C++20.
> But as no one uses the constexpr version, the compiler doesn't tell.
> 
> Then I realized that a lot of the operators are declared constexpr even
> though they are not constexpr. (Because the inner transform is constexpr
> since C++20).
> 
> Removing all those constexpr without being able to test them is not
> really sensible. So my current strategy on this is:
> 
> The constexpr declaration is merely an "intent" as we can't test it. We
> might need to fix things when we need it, or not if we switch to C++20
> first. Taking that direction I should leave the constexpr in place in
> this case although I know it is not working. Oh that doesn't feel right
> either...
> 
> Opinions?

I'm fine with this. You're not making things worse, and we're not
cornering ourselves in a bad way, so we can wait to see if we'll switch
to C++20 first or will replace std::copy.

> > > +
> > > +       Vector(const Span<const T, Rows> &data)
> > > +       {
> > > +               std::copy(data.begin(), data.end(), data_.begin());
> > >         }
> > >  
> > >         const T &operator[](size_t i) const
> > > @@ -285,7 +289,7 @@ private:
> > >                 return *this;
> > >         }
> > >  
> > > -       std::array<T, Rows> data_;
> > > +       std::array<T, Rows> data_{};
> > 
> > Is this just for initialisation ?
> 
> Yes, I added a comment in v3.
> 
> Best regards,
> Stefan
> 
> > >  };
> > >  
> > >  template<typename T>
> > > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp
> > > index 85ca2208245a..435f2fc62f8b 100644
> > > --- a/src/libcamera/vector.cpp
> > > +++ b/src/libcamera/vector.cpp
> > > @@ -44,6 +44,14 @@ LOG_DEFINE_CATEGORY(Vector)
> > >   * The size of \a data must be equal to the dimension size Rows of the vector.
> > >   */
> > >  
> > > +/**
> > > + * \fn Vector::Vector(const Span<const T, Rows> &data)
> > > + * \brief Construct vector from supplied data
> > > + * \param data Data from which to construct a vector
> > > + *
> > > + * The size of \a data must be equal to the dimension size Rows of the vector.
> > > + */
> > > +
> > >  /**
> > >   * \fn T Vector::operator[](size_t i) const
> > >   * \brief Index to an element in the vector

-- 
Regards,

Laurent Pinchart