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

22982 diff mbox series

Now that Matrix has a Span based constructor, add the same for Vector.

Signed-off-by: Stefan Klug <stefan.klug@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(-)

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

I assume Vectors deserve span based constructors because it benefits
them directly?


> 
> Signed-off-by: Stefan Klug <stefan.klug@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.


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

>  };
>  
>  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
> -- 
> 2.43.0
>

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 ...
> :-)
> 
> I assume Vectors deserve span based constructors because it benefits
> them directly?

A similar commit message to 03/17 would be better indeed.

All my comments on 03/17 apply here too.

> > Signed-off-by: Stefan Klug <stefan.klug@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.
> 
> > +
> > +       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 ?
>
> >  };
> >  
> >  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

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

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

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