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

[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 at 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 at ideasonboard.com>
> > Reviewed-by: Milan Zamazal <mzamazal at 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;

-- 
Regards,

Laurent Pinchart