[v3,04/17] ipa: libipa: vector: Add copy constructor and assignment operator
diff mbox series

Message ID 20241118221618.13953-5-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Improve linear algebra helpers in libipa
Related show

Commit Message

Laurent Pinchart Nov. 18, 2024, 10:16 p.m. UTC 
It is useful to assign a value to an existing vector. Define a copy
constructor and a copy assignment operator.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
---
Changes since v2:

- Make copy constructor constexpr
- Drop #include <algorithm>
---
 src/ipa/libipa/vector.cpp | 13 +++++++++++++
 src/ipa/libipa/vector.h   | 12 ++++++++++++
 2 files changed, 25 insertions(+)

Comments

Barnabás Pőcze Nov. 18, 2024, 11:30 p.m. UTC | #1 
Hi


2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> It is useful to assign a value to an existing vector. Define a copy
> constructor and a copy assignment operator.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> ---
> Changes since v2:
> 
> - Make copy constructor constexpr
> - Drop #include <algorithm>
> ---
>  src/ipa/libipa/vector.cpp | 13 +++++++++++++
>  src/ipa/libipa/vector.h   | 12 ++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> index d414ba97e41e..80f1758578f0 100644
> --- a/src/ipa/libipa/vector.cpp
> +++ b/src/ipa/libipa/vector.cpp
> @@ -46,6 +46,19 @@ namespace ipa {
>   * The size of \a data must be equal to the dimension size Rows of the vector.
>   */
> 
> +/**
> + * \fn Vector::Vector(const Vector &other)
> + * \brief Construct a Vector by copying \a other
> + * \param[in] other The other Vector value
> + */
> +
> +/**
> + * \fn Vector &Vector::operator=(const Vector &other)
> + * \brief Replace the content of the Vector with a copy of the content of \a other
> + * \param[in] other The other Vector value
> + * \return This Vector value
> + */
> +
>  /**
>   * \fn T Vector::operator[](size_t i) const
>   * \brief Index to an element in the vector
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> index be568eadfeac..35fc9539dc99 100644
> --- a/src/ipa/libipa/vector.h
> +++ b/src/ipa/libipa/vector.h
> @@ -46,6 +46,18 @@ public:
>  			data_[i] = data[i];
>  	}
> 
> +	constexpr Vector(const Vector &other)
> +		: data_(other.data_)
> +	{
> +	}
> +
> +	Vector &operator=(const Vector &other)
> +	{
> +		data_ = other.data_;
> +
> +		return *this;
> +	}

Why not `... = default` in both cases? It seems to me that
the compiler generated functions would be appropriate.


Regards,
Barnabás Pőcze


> +
>  	const T &operator[](size_t i) const
>  	{
>  		ASSERT(i < data_.size());
> --
> Regards,
> 
> Laurent Pinchart
> 
>
Laurent Pinchart Nov. 19, 2024, 8:38 a.m. UTC | #2 
Hi Barnabás,

On Mon, Nov 18, 2024 at 11:30:12PM +0000, Barnabás Pőcze wrote:
> 2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart írta:
> 
> > It is useful to assign a value to an existing vector. Define a copy
> > constructor and a copy assignment operator.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > ---
> > Changes since v2:
> > 
> > - Make copy constructor constexpr
> > - Drop #include <algorithm>
> > ---
> >  src/ipa/libipa/vector.cpp | 13 +++++++++++++
> >  src/ipa/libipa/vector.h   | 12 ++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > index d414ba97e41e..80f1758578f0 100644
> > --- a/src/ipa/libipa/vector.cpp
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -46,6 +46,19 @@ namespace ipa {
> >   * The size of \a data must be equal to the dimension size Rows of the vector.
> >   */
> > 
> > +/**
> > + * \fn Vector::Vector(const Vector &other)
> > + * \brief Construct a Vector by copying \a other
> > + * \param[in] other The other Vector value
> > + */
> > +
> > +/**
> > + * \fn Vector &Vector::operator=(const Vector &other)
> > + * \brief Replace the content of the Vector with a copy of the content of \a other
> > + * \param[in] other The other Vector value
> > + * \return This Vector value
> > + */
> > +
> >  /**
> >   * \fn T Vector::operator[](size_t i) const
> >   * \brief Index to an element in the vector
> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > index be568eadfeac..35fc9539dc99 100644
> > --- a/src/ipa/libipa/vector.h
> > +++ b/src/ipa/libipa/vector.h
> > @@ -46,6 +46,18 @@ public:
> >  			data_[i] = data[i];
> >  	}
> > 
> > +	constexpr Vector(const Vector &other)
> > +		: data_(other.data_)
> > +	{
> > +	}
> > +
> > +	Vector &operator=(const Vector &other)
> > +	{
> > +		data_ = other.data_;
> > +
> > +		return *this;
> > +	}
> 
> Why not `... = default` in both cases? It seems to me that
> the compiler generated functions would be appropriate.

I will give it a try in v4.

> > +
> >  	const T &operator[](size_t i) const
> >  	{
> >  		ASSERT(i < data_.size());
Barnabás Pőcze Nov. 19, 2024, 2:07 p.m. UTC | #3 
Hi


2024. november 19., kedd 9:38 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> Hi Barnabás,
> 
> On Mon, Nov 18, 2024 at 11:30:12PM +0000, Barnabás Pőcze wrote:
> > 2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart írta:
> >
> > > It is useful to assign a value to an existing vector. Define a copy
> > > constructor and a copy assignment operator.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > > ---
> > > Changes since v2:
> > >
> > > - Make copy constructor constexpr
> > > - Drop #include <algorithm>
> > > ---
> > >  src/ipa/libipa/vector.cpp | 13 +++++++++++++
> > >  src/ipa/libipa/vector.h   | 12 ++++++++++++
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > > index d414ba97e41e..80f1758578f0 100644
> > > --- a/src/ipa/libipa/vector.cpp
> > > +++ b/src/ipa/libipa/vector.cpp
> > > @@ -46,6 +46,19 @@ namespace ipa {
> > >   * The size of \a data must be equal to the dimension size Rows of the vector.
> > >   */
> > >
> > > +/**
> > > + * \fn Vector::Vector(const Vector &other)
> > > + * \brief Construct a Vector by copying \a other
> > > + * \param[in] other The other Vector value
> > > + */
> > > +
> > > +/**
> > > + * \fn Vector &Vector::operator=(const Vector &other)
> > > + * \brief Replace the content of the Vector with a copy of the content of \a other
> > > + * \param[in] other The other Vector value
> > > + * \return This Vector value
> > > + */
> > > +
> > >  /**
> > >   * \fn T Vector::operator[](size_t i) const
> > >   * \brief Index to an element in the vector
> > > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > > index be568eadfeac..35fc9539dc99 100644
> > > --- a/src/ipa/libipa/vector.h
> > > +++ b/src/ipa/libipa/vector.h
> > > @@ -46,6 +46,18 @@ public:
> > >  			data_[i] = data[i];
> > >  	}
> > >
> > > +	constexpr Vector(const Vector &other)
> > > +		: data_(other.data_)
> > > +	{
> > > +	}
> > > +
> > > +	Vector &operator=(const Vector &other)
> > > +	{
> > > +		data_ = other.data_;
> > > +
> > > +		return *this;
> > > +	}
> >
> > Why not `... = default` in both cases? It seems to me that
> > the compiler generated functions would be appropriate.
> 
> I will give it a try in v4.

Sorry, I have just now realized that these functions should already be implicitly
defined with the ` = default` definition, so there is probably no need to define
them at all unless I am missing something.

Regards,
Barnabás Pőcze

> 
> > > +
> > >  	const T &operator[](size_t i) const
> > >  	{
> > >  		ASSERT(i < data_.size());
> 
> --
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Nov. 19, 2024, 4:41 p.m. UTC | #4 
On Tue, Nov 19, 2024 at 02:07:24PM +0000, Barnabás Pőcze wrote:
> 2024. november 19., kedd 9:38 keltezéssel, Laurent Pinchart írta:
> > On Mon, Nov 18, 2024 at 11:30:12PM +0000, Barnabás Pőcze wrote:
> > > 2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart írta:
> > >
> > > > It is useful to assign a value to an existing vector. Define a copy
> > > > constructor and a copy assignment operator.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > > > ---
> > > > Changes since v2:
> > > >
> > > > - Make copy constructor constexpr
> > > > - Drop #include <algorithm>
> > > > ---
> > > >  src/ipa/libipa/vector.cpp | 13 +++++++++++++
> > > >  src/ipa/libipa/vector.h   | 12 ++++++++++++
> > > >  2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > > > index d414ba97e41e..80f1758578f0 100644
> > > > --- a/src/ipa/libipa/vector.cpp
> > > > +++ b/src/ipa/libipa/vector.cpp
> > > > @@ -46,6 +46,19 @@ namespace ipa {
> > > >   * The size of \a data must be equal to the dimension size Rows of the vector.
> > > >   */
> > > >
> > > > +/**
> > > > + * \fn Vector::Vector(const Vector &other)
> > > > + * \brief Construct a Vector by copying \a other
> > > > + * \param[in] other The other Vector value
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Vector &Vector::operator=(const Vector &other)
> > > > + * \brief Replace the content of the Vector with a copy of the content of \a other
> > > > + * \param[in] other The other Vector value
> > > > + * \return This Vector value
> > > > + */
> > > > +
> > > >  /**
> > > >   * \fn T Vector::operator[](size_t i) const
> > > >   * \brief Index to an element in the vector
> > > > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > > > index be568eadfeac..35fc9539dc99 100644
> > > > --- a/src/ipa/libipa/vector.h
> > > > +++ b/src/ipa/libipa/vector.h
> > > > @@ -46,6 +46,18 @@ public:
> > > >  			data_[i] = data[i];
> > > >  	}
> > > >
> > > > +	constexpr Vector(const Vector &other)
> > > > +		: data_(other.data_)
> > > > +	{
> > > > +	}
> > > > +
> > > > +	Vector &operator=(const Vector &other)
> > > > +	{
> > > > +		data_ = other.data_;
> > > > +
> > > > +		return *this;
> > > > +	}
> > >
> > > Why not `... = default` in both cases? It seems to me that
> > > the compiler generated functions would be appropriate.
> > 
> > I will give it a try in v4.
> 
> Sorry, I have just now realized that these functions should already be implicitly
> defined with the ` = default` definition, so there is probably no need to define
> them at all unless I am missing something.

You're right. I'll just drop this patch.

> > > > +
> > > >  	const T &operator[](size_t i) const
> > > >  	{
> > > >  		ASSERT(i < data_.size());

Patch
diff mbox series 

diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
index d414ba97e41e..80f1758578f0 100644
--- a/src/ipa/libipa/vector.cpp
+++ b/src/ipa/libipa/vector.cpp
@@ -46,6 +46,19 @@  namespace ipa {
  * The size of \a data must be equal to the dimension size Rows of the vector.
  */
 
+/**
+ * \fn Vector::Vector(const Vector &other)
+ * \brief Construct a Vector by copying \a other
+ * \param[in] other The other Vector value
+ */
+
+/**
+ * \fn Vector &Vector::operator=(const Vector &other)
+ * \brief Replace the content of the Vector with a copy of the content of \a other
+ * \param[in] other The other Vector value
+ * \return This Vector value
+ */
+
 /**
  * \fn T Vector::operator[](size_t i) const
  * \brief Index to an element in the vector
diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
index be568eadfeac..35fc9539dc99 100644
--- a/src/ipa/libipa/vector.h
+++ b/src/ipa/libipa/vector.h
@@ -46,6 +46,18 @@  public:
 			data_[i] = data[i];
 	}
 
+	constexpr Vector(const Vector &other)
+		: data_(other.data_)
+	{
+	}
+
+	Vector &operator=(const Vector &other)
+	{
+		data_ = other.data_;
+
+		return *this;
+	}
+
 	const T &operator[](size_t i) const
 	{
 		ASSERT(i < data_.size());