[v3,01/17] ipa: libipa: vector: Add mutable x(), y() and z() accessors

21961 diff mbox series

The x(), y() and z() functions of the Vector class are convenience
accessors for the first, second and third element of the vector
respectively, meant to improve readability of class users when a vector
represents coordinates in 1D, 2D or 3D space. Those accessors are
limited to immutable access to the vector elements, as they return a
copy. Extend the API with mutable accessors.

The immutable accessors are modified to return a reference to the vector
elements instead of a copy for consistency. As they are inline
functions, this should make no difference in terms of performance as the
compiler can perform the same optimizations in their case.

While at it, reorder functions to declare operators before other member
functions, to be consistent with the usual coding style.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/libipa/vector.cpp | 51 +++++++++++++++++++++++++--------------
 src/ipa/libipa/vector.h   | 49 +++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 42 deletions(-)

Hi


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

> The x(), y() and z() functions of the Vector class are convenience
> accessors for the first, second and third element of the vector
> respectively, meant to improve readability of class users when a vector
> represents coordinates in 1D, 2D or 3D space. Those accessors are
> limited to immutable access to the vector elements, as they return a
> copy. Extend the API with mutable accessors.
> 
> The immutable accessors are modified to return a reference to the vector
> elements instead of a copy for consistency. As they are inline
> functions, this should make no difference in terms of performance as the
> compiler can perform the same optimizations in their case.
> 
> While at it, reorder functions to declare operators before other member
> functions, to be consistent with the usual coding style.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/libipa/vector.cpp | 51 +++++++++++++++++++++++++--------------
>  src/ipa/libipa/vector.h   | 49 +++++++++++++++++++------------------
>  2 files changed, 58 insertions(+), 42 deletions(-)
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> index bd00b01961d5..8a39bfd27f90 100644
> --- a/src/ipa/libipa/vector.cpp
> +++ b/src/ipa/libipa/vector.cpp
> @@ -52,24 +52,6 @@ namespace ipa {
>   * \copydoc Vector::operator[](size_t i) const
>   */
> 
> -/**
> - * \fn Vector::x()
> - * \brief Convenience function to access the first element of the vector
> - * \return The first element of the vector
> - */
> -
> -/**
> - * \fn Vector::y()
> - * \brief Convenience function to access the second element of the vector
> - * \return The second element of the vector
> - */
> -
> -/**
> - * \fn Vector::z()
> - * \brief Convenience function to access the third element of the vector
> - * \return The third element of the vector
> - */
> -
>  /**
>   * \fn Vector::operator-() const
>   * \brief Negate a Vector by negating both all of its coordinates
> @@ -111,6 +93,39 @@ namespace ipa {
>   * \return The vector divided by \a factor
>   */
> 
> +/**
> + * \fn T &Vector::x()
> + * \brief Convenience function to access the first element of the vector
> + * \return The first element of the vector
> + */
> +
> +/**
> + * \fn T &Vector::y()
> + * \brief Convenience function to access the second element of the vector
> + * \return The second element of the vector
> + */
> +
> +/**
> + * \fn T &Vector::z()
> + * \brief Convenience function to access the third element of the vector
> + * \return The third element of the vector
> + */
> +
> +/**
> + * \fn constexpr const T &Vector::x() const
> + * \copydoc Vector::x()
> + */
> +
> +/**
> + * \fn constexpr const T &Vector::y() const
> + * \copydoc Vector::y()
> + */
> +
> +/**
> + * \fn constexpr const T &Vector::z() const
> + * \copydoc Vector::z()
> + */
> +
>  /**
>   * \fn Vector::length2()
>   * \brief Get the squared length of the vector
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> index 8612a06a2ab2..1b11a34deee4 100644
> --- a/src/ipa/libipa/vector.h
> +++ b/src/ipa/libipa/vector.h
> @@ -53,30 +53,6 @@ public:
>  		return data_[i];
>  	}
> 
> -#ifndef __DOXYGEN__
> -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> -#endif /* __DOXYGEN__ */
> -	constexpr T x() const
> -	{
> -		return data_[0];
> -	}
> -
> -#ifndef __DOXYGEN__
> -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> -#endif /* __DOXYGEN__ */
> -	constexpr T y() const
> -	{
> -		return data_[1];
> -	}
> -
> -#ifndef __DOXYGEN__
> -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> -#endif /* __DOXYGEN__ */
> -	constexpr T z() const
> -	{
> -		return data_[2];
> -	}
> -
>  	constexpr Vector<T, Rows> operator-() const
>  	{
>  		Vector<T, Rows> ret;
> @@ -125,6 +101,31 @@ public:
>  		return ret;
>  	}
> 
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> +#endif /* __DOXYGEN__ */
> +	constexpr const T &x() const { return data_[0]; }
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> +#endif /* __DOXYGEN__ */
> +	constexpr const T &y() const { return data_[1]; }
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> +#endif /* __DOXYGEN__ */
> +	constexpr const T &z() const { return data_[2]; }
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> +#endif /* __DOXYGEN__ */
> +	T &x() { return data_[0]; }
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> +#endif /* __DOXYGEN__ */
> +	T &y() { return data_[1]; }
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> +#endif /* __DOXYGEN__ */
> +	T &z() { return data_[2]; }

I see no reason not to make the non-const qualified versions `constexpr` as well.
Same with r(), g(), b() in the next patch.


Regards,
Barnabás Pőcze

> +
>  	constexpr double length2() const
>  	{
>  		double ret = 0;
> --
> Regards,
> 
> Laurent Pinchart

On Tue, Nov 19, 2024 at 03:28:25AM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:
> 
> > The x(), y() and z() functions of the Vector class are convenience
> > accessors for the first, second and third element of the vector
> > respectively, meant to improve readability of class users when a vector
> > represents coordinates in 1D, 2D or 3D space. Those accessors are
> > limited to immutable access to the vector elements, as they return a
> > copy. Extend the API with mutable accessors.
> > 
> > The immutable accessors are modified to return a reference to the vector
> > elements instead of a copy for consistency. As they are inline
> > functions, this should make no difference in terms of performance as the
> > compiler can perform the same optimizations in their case.
> > 
> > While at it, reorder functions to declare operators before other member
> > functions, to be consistent with the usual coding style.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> > ---
> >  src/ipa/libipa/vector.cpp | 51 +++++++++++++++++++++++++--------------
> >  src/ipa/libipa/vector.h   | 49 +++++++++++++++++++------------------
> >  2 files changed, 58 insertions(+), 42 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > index bd00b01961d5..8a39bfd27f90 100644
> > --- a/src/ipa/libipa/vector.cpp
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -52,24 +52,6 @@ namespace ipa {
> >   * \copydoc Vector::operator[](size_t i) const
> >   */
> > 
> > -/**
> > - * \fn Vector::x()
> > - * \brief Convenience function to access the first element of the vector
> > - * \return The first element of the vector
> > - */
> > -
> > -/**
> > - * \fn Vector::y()
> > - * \brief Convenience function to access the second element of the vector
> > - * \return The second element of the vector
> > - */
> > -
> > -/**
> > - * \fn Vector::z()
> > - * \brief Convenience function to access the third element of the vector
> > - * \return The third element of the vector
> > - */
> > -
> >  /**
> >   * \fn Vector::operator-() const
> >   * \brief Negate a Vector by negating both all of its coordinates
> > @@ -111,6 +93,39 @@ namespace ipa {
> >   * \return The vector divided by \a factor
> >   */
> > 
> > +/**
> > + * \fn T &Vector::x()
> > + * \brief Convenience function to access the first element of the vector
> > + * \return The first element of the vector
> > + */
> > +
> > +/**
> > + * \fn T &Vector::y()
> > + * \brief Convenience function to access the second element of the vector
> > + * \return The second element of the vector
> > + */
> > +
> > +/**
> > + * \fn T &Vector::z()
> > + * \brief Convenience function to access the third element of the vector
> > + * \return The third element of the vector
> > + */
> > +
> > +/**
> > + * \fn constexpr const T &Vector::x() const
> > + * \copydoc Vector::x()
> > + */
> > +
> > +/**
> > + * \fn constexpr const T &Vector::y() const
> > + * \copydoc Vector::y()
> > + */
> > +
> > +/**
> > + * \fn constexpr const T &Vector::z() const
> > + * \copydoc Vector::z()
> > + */
> > +
> >  /**
> >   * \fn Vector::length2()
> >   * \brief Get the squared length of the vector
> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > index 8612a06a2ab2..1b11a34deee4 100644
> > --- a/src/ipa/libipa/vector.h
> > +++ b/src/ipa/libipa/vector.h
> > @@ -53,30 +53,6 @@ public:
> >  		return data_[i];
> >  	}
> > 
> > -#ifndef __DOXYGEN__
> > -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> > -#endif /* __DOXYGEN__ */
> > -	constexpr T x() const
> > -	{
> > -		return data_[0];
> > -	}
> > -
> > -#ifndef __DOXYGEN__
> > -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> > -#endif /* __DOXYGEN__ */
> > -	constexpr T y() const
> > -	{
> > -		return data_[1];
> > -	}
> > -
> > -#ifndef __DOXYGEN__
> > -	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> > -#endif /* __DOXYGEN__ */
> > -	constexpr T z() const
> > -	{
> > -		return data_[2];
> > -	}
> > -
> >  	constexpr Vector<T, Rows> operator-() const
> >  	{
> >  		Vector<T, Rows> ret;
> > @@ -125,6 +101,31 @@ public:
> >  		return ret;
> >  	}
> > 
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> > +#endif /* __DOXYGEN__ */
> > +	constexpr const T &x() const { return data_[0]; }
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> > +#endif /* __DOXYGEN__ */
> > +	constexpr const T &y() const { return data_[1]; }
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> > +#endif /* __DOXYGEN__ */
> > +	constexpr const T &z() const { return data_[2]; }
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> > +#endif /* __DOXYGEN__ */
> > +	T &x() { return data_[0]; }
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> > +#endif /* __DOXYGEN__ */
> > +	T &y() { return data_[1]; }
> > +#ifndef __DOXYGEN__
> > +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> > +#endif /* __DOXYGEN__ */
> > +	T &z() { return data_[2]; }
> 
> I see no reason not to make the non-const qualified versions `constexpr` as well.
> Same with r(), g(), b() in the next patch.

I didn't think there would be a use case for a constexpr on a non-const
member function, and then found this little gem:

$ cat <<EOF > constexpr.cpp
template<int I>
struct Foo
{
        static constexpr int i = I;
};

struct A
{
        constexpr A() = default;

        constexpr int value()
        {
                return value_;
        }

        constexpr A &increment()
        {
                ++value_;
                return *this;
        }

private:
        int value_ = 42;
};

int main()
{
        Foo<A{}.increment().value()> foo;

        return foo.i;
}
EOF
$ g++ -W -Wall -O2 -o constexpr constexpr.cpp
$ ./constexpr ; echo $?
43
$ objdump --disassemble=main constexpr

constexpr:     file format elf64-x86-64


Disassembly of section .init:

Disassembly of section .plt:

Disassembly of section .plt.got:

Disassembly of section .text:

0000000000001040 <main>:
    1040:       f3 0f 1e fa             endbr64
    1044:       b8 2b 00 00 00          mov    $0x2b,%eax
    1049:       c3                      ret

Disassembly of section .fini:


C++ is amazing (the definition of "amazing" depends on who you ask).

I'll make those functions constexpr.

> > +
> >  	constexpr double length2() const
> >  	{
> >  		double ret = 0;