[{"id":32248,"web_url":"https://patchwork.libcamera.org/comment/32248/","msgid":"<5cKEjoyO3lndE_ROm1YxerxiIV766m82rHqrsU01TYl07jQlhJGgdsfhkXduWT58Aw5SHh74X_tKRAaVvSetK2GTDD3JtbTg4QS6QP7kaUY=@protonmail.com>","date":"2024-11-19T03:28:25","subject":"Re: [PATCH v3 01/17] ipa: libipa: vector: Add mutable x(),\n\ty() and z() accessors","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart írta:\n\n> The x(), y() and z() functions of the Vector class are convenience\n> accessors for the first, second and third element of the vector\n> respectively, meant to improve readability of class users when a vector\n> represents coordinates in 1D, 2D or 3D space. Those accessors are\n> limited to immutable access to the vector elements, as they return a\n> copy. Extend the API with mutable accessors.\n> \n> The immutable accessors are modified to return a reference to the vector\n> elements instead of a copy for consistency. As they are inline\n> functions, this should make no difference in terms of performance as the\n> compiler can perform the same optimizations in their case.\n> \n> While at it, reorder functions to declare operators before other member\n> functions, to be consistent with the usual coding style.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Milan Zamazal \n> ---\n> src/ipa/libipa/vector.cpp | 51 +++++++++++++++++++++++++--------------\n> src/ipa/libipa/vector.h | 49 +++++++++++++++++++------------------\n> 2 files changed, 58 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> index bd00b01961d5..8a39bfd27f90 100644\n> --- a/src/ipa/libipa/vector.cpp\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -52,24 +52,6 @@ namespace ipa {\n> * \\copydoc Vector::operator[](size_t i) const\n> */\n> \n> -/**\n> - * \\fn Vector::x()\n> - * \\brief Convenience function to access the first element of the vector\n> - * \\return The first element of the vector\n> - */\n> -\n> -/**\n> - * \\fn Vector::y()\n> - * \\brief Convenience function to access the second element of the vector\n> - * \\return The second element of the vector\n> - */\n> -\n> -/**\n> - * \\fn Vector::z()\n> - * \\brief Convenience function to access the third element of the vector\n> - * \\return The third element of the vector\n> - */\n> -\n> /**\n> * \\fn Vector::operator-() const\n> * \\brief Negate a Vector by negating both all of its coordinates\n> @@ -111,6 +93,39 @@ namespace ipa {\n> * \\return The vector divided by \\a factor\n> */\n> \n> +/**\n> + * \\fn T &Vector::x()\n> + * \\brief Convenience function to access the first element of the vector\n> + * \\return The first element of the vector\n> + */\n> +\n> +/**\n> + * \\fn T &Vector::y()\n> + * \\brief Convenience function to access the second element of the vector\n> + * \\return The second element of the vector\n> + */\n> +\n> +/**\n> + * \\fn T &Vector::z()\n> + * \\brief Convenience function to access the third element of the vector\n> + * \\return The third element of the vector\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::x() const\n> + * \\copydoc Vector::x()\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::y() const\n> + * \\copydoc Vector::y()\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::z() const\n> + * \\copydoc Vector::z()\n> + */\n> +\n> /**\n> * \\fn Vector::length2()\n> * \\brief Get the squared length of the vector\n> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> index 8612a06a2ab2..1b11a34deee4 100644\n> --- a/src/ipa/libipa/vector.h\n> +++ b/src/ipa/libipa/vector.h\n> @@ -53,30 +53,6 @@ public:\n> \t\treturn data_[i];\n> \t}\n> \n> -#ifndef __DOXYGEN__\n> -\ttemplate= 1>>\n> -#endif /* __DOXYGEN__ */\n> -\tconstexpr T x() const\n> -\t{\n> -\t\treturn data_[0];\n> -\t}\n> -\n> -#ifndef __DOXYGEN__\n> -\ttemplate= 2>>\n> -#endif /* __DOXYGEN__ */\n> -\tconstexpr T y() const\n> -\t{\n> -\t\treturn data_[1];\n> -\t}\n> -\n> -#ifndef __DOXYGEN__\n> -\ttemplate= 3>>\n> -#endif /* __DOXYGEN__ */\n> -\tconstexpr T z() const\n> -\t{\n> -\t\treturn data_[2];\n> -\t}\n> -\n> \tconstexpr Vector operator-() const\n> \t{\n> \t\tVector ret;\n> @@ -125,6 +101,31 @@ public:\n> \t\treturn ret;\n> \t}\n> \n> +#ifndef __DOXYGEN__\n> +\ttemplate= 1>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &x() const { return data_[0]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate= 2>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &y() const { return data_[1]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate= 3>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &z() const { return data_[2]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate= 1>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &x() { return data_[0]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate= 2>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &y() { return data_[1]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate= 3>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &z() { return data_[2]; }\n\nI see no reason not to make the non-const qualified versions `constexpr` as well.\nSame with r(), g(), b() in the next patch.\n\n\nRegards,\nBarnabás Pőcze\n\n> +\n> \tconstexpr double length2() const\n> \t{\n> \t\tdouble ret = 0;\n> --\n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6BF42C32F1\n\tfor ;\n\tTue, 19 Nov 2024 03:28:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94BF1658FD;\n\tTue, 19 Nov 2024 04:28:32 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CC5062C8C\n\tfor ;\n\tTue, 19 Nov 2024 04:28:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"XlRUpEb0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1731986909; x=1732246109;\n\tbh=ymAzZseDY9FhVip9fmjJN/dmI4P7BaFl8YwZOZetGQ4=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=XlRUpEb0UVWhY64qf1TZylDdhVPWyo3e262KEQODLmxvY7abzuWlk+a3mwYNUxvfq\n\tcUZyVzNhQrCkGrId4N6mnsl6NiWehRmdgu/veRz0CfrlWwOY7z69ZcIlTVY1FTKD8q\n\tDMI99M5r6z9oEZIFAJ46OFvV1dAeu+VejZBIMYiaZKEQKng/QlqlHebygpm3su2JmO\n\tOfoa6nTzKTAGQgjyyb0uqg1bBJInEwK/2FwjHTesjf4R41fIOi8ZOg9MCvtj1LgJNT\n\tsyTPSSEeF1JOi3X+FRaxuMPOG4GieOnkbXVucOpLco+onRPMWNeCUvDEu2tqCT5udJ\n\thrYmCILjURwOg==","Date":"Tue, 19 Nov 2024 03:28:25 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 01/17] ipa: libipa: vector: Add mutable x(),\n\ty() and z() accessors","Message-ID":"<5cKEjoyO3lndE_ROm1YxerxiIV766m82rHqrsU01TYl07jQlhJGgdsfhkXduWT58Aw5SHh74X_tKRAaVvSetK2GTDD3JtbTg4QS6QP7kaUY=@protonmail.com>","In-Reply-To":"<20241118221618.13953-2-laurent.pinchart@ideasonboard.com>","References":"<20241118221618.13953-1-laurent.pinchart@ideasonboard.com>\n\t<20241118221618.13953-2-laurent.pinchart@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"62376673ed6a865891160a16168ca624327edde6","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32252,"web_url":"https://patchwork.libcamera.org/comment/32252/","msgid":"<20241119081729.GB31681@pendragon.ideasonboard.com>","date":"2024-11-19T08:17:29","subject":"Re: [PATCH v3 01/17] ipa: libipa: vector: Add mutable x(), y() and\n\tz() accessors","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 19, 2024 at 03:28:25AM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2024. november 18., hétfő 23:16 keltezéssel, Laurent Pinchart írta:\n> \n> > The x(), y() and z() functions of the Vector class are convenience\n> > accessors for the first, second and third element of the vector\n> > respectively, meant to improve readability of class users when a vector\n> > represents coordinates in 1D, 2D or 3D space. Those accessors are\n> > limited to immutable access to the vector elements, as they return a\n> > copy. Extend the API with mutable accessors.\n> > \n> > The immutable accessors are modified to return a reference to the vector\n> > elements instead of a copy for consistency. As they are inline\n> > functions, this should make no difference in terms of performance as the\n> > compiler can perform the same optimizations in their case.\n> > \n> > While at it, reorder functions to declare operators before other member\n> > functions, to be consistent with the usual coding style.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Milan Zamazal \n> > ---\n> > src/ipa/libipa/vector.cpp | 51 +++++++++++++++++++++++++--------------\n> > src/ipa/libipa/vector.h | 49 +++++++++++++++++++------------------\n> > 2 files changed, 58 insertions(+), 42 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> > index bd00b01961d5..8a39bfd27f90 100644\n> > --- a/src/ipa/libipa/vector.cpp\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -52,24 +52,6 @@ namespace ipa {\n> > * \\copydoc Vector::operator[](size_t i) const\n> > */\n> > \n> > -/**\n> > - * \\fn Vector::x()\n> > - * \\brief Convenience function to access the first element of the vector\n> > - * \\return The first element of the vector\n> > - */\n> > -\n> > -/**\n> > - * \\fn Vector::y()\n> > - * \\brief Convenience function to access the second element of the vector\n> > - * \\return The second element of the vector\n> > - */\n> > -\n> > -/**\n> > - * \\fn Vector::z()\n> > - * \\brief Convenience function to access the third element of the vector\n> > - * \\return The third element of the vector\n> > - */\n> > -\n> > /**\n> > * \\fn Vector::operator-() const\n> > * \\brief Negate a Vector by negating both all of its coordinates\n> > @@ -111,6 +93,39 @@ namespace ipa {\n> > * \\return The vector divided by \\a factor\n> > */\n> > \n> > +/**\n> > + * \\fn T &Vector::x()\n> > + * \\brief Convenience function to access the first element of the vector\n> > + * \\return The first element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T &Vector::y()\n> > + * \\brief Convenience function to access the second element of the vector\n> > + * \\return The second element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T &Vector::z()\n> > + * \\brief Convenience function to access the third element of the vector\n> > + * \\return The third element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::x() const\n> > + * \\copydoc Vector::x()\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::y() const\n> > + * \\copydoc Vector::y()\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::z() const\n> > + * \\copydoc Vector::z()\n> > + */\n> > +\n> > /**\n> > * \\fn Vector::length2()\n> > * \\brief Get the squared length of the vector\n> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> > index 8612a06a2ab2..1b11a34deee4 100644\n> > --- a/src/ipa/libipa/vector.h\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -53,30 +53,6 @@ public:\n> > \t\treturn data_[i];\n> > \t}\n> > \n> > -#ifndef __DOXYGEN__\n> > -\ttemplate= 1>>\n> > -#endif /* __DOXYGEN__ */\n> > -\tconstexpr T x() const\n> > -\t{\n> > -\t\treturn data_[0];\n> > -\t}\n> > -\n> > -#ifndef __DOXYGEN__\n> > -\ttemplate= 2>>\n> > -#endif /* __DOXYGEN__ */\n> > -\tconstexpr T y() const\n> > -\t{\n> > -\t\treturn data_[1];\n> > -\t}\n> > -\n> > -#ifndef __DOXYGEN__\n> > -\ttemplate= 3>>\n> > -#endif /* __DOXYGEN__ */\n> > -\tconstexpr T z() const\n> > -\t{\n> > -\t\treturn data_[2];\n> > -\t}\n> > -\n> > \tconstexpr Vector operator-() const\n> > \t{\n> > \t\tVector ret;\n> > @@ -125,6 +101,31 @@ public:\n> > \t\treturn ret;\n> > \t}\n> > \n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 1>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &x() const { return data_[0]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 2>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &y() const { return data_[1]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 3>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &z() const { return data_[2]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 1>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &x() { return data_[0]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 2>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &y() { return data_[1]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate= 3>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &z() { return data_[2]; }\n> \n> I see no reason not to make the non-const qualified versions `constexpr` as well.\n> Same with r(), g(), b() in the next patch.\n\nI didn't think there would be a use case for a constexpr on a non-const\nmember function, and then found this little gem:\n\n$ cat < constexpr.cpp\ntemplate\nstruct Foo\n{\n static constexpr int i = I;\n};\n\nstruct A\n{\n constexpr A() = default;\n\n constexpr int value()\n {\n return value_;\n }\n\n constexpr A &increment()\n {\n ++value_;\n return *this;\n }\n\nprivate:\n int value_ = 42;\n};\n\nint main()\n{\n Foo foo;\n\n return foo.i;\n}\nEOF\n$ g++ -W -Wall -O2 -o constexpr constexpr.cpp\n$ ./constexpr ; echo $?\n43\n$ objdump --disassemble=main constexpr\n\nconstexpr: file format elf64-x86-64\n\n\nDisassembly of section .init:\n\nDisassembly of section .plt:\n\nDisassembly of section .plt.got:\n\nDisassembly of section .text:\n\n0000000000001040
:\n 1040: f3 0f 1e fa endbr64\n 1044: b8 2b 00 00 00 mov $0x2b,%eax\n 1049: c3 ret\n\nDisassembly of section .fini:\n\n\nC++ is amazing (the definition of \"amazing\" depends on who you ask).\n\nI'll make those functions constexpr.\n\n> > +\n> > \tconstexpr double length2() const\n> > \t{\n> > \t\tdouble ret = 0;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6224CC32F1\n\tfor ;\n\tTue, 19 Nov 2024 08:17:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EDD46590C;\n\tTue, 19 Nov 2024 09:17:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FBB265898\n\tfor ;\n\tTue, 19 Nov 2024 09:17:38 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87F43520;\n\tTue, 19 Nov 2024 09:17:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WyrMd/Gt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732004240;\n\tbh=ixGzB2b8SopRMpL/Xvny9uLAazD0sSxUs1IW91FSTHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WyrMd/GtK+9MziBsEQ7I1Bh7gNO0o21/DHf7odESosVNv7S22dOiulSASm45x1u2A\n\tVJIkJ7zmvRULBG0c/jUWMEU0Z9771FRooEWbMTqViZLJWPiL/LL9CtivNV9uJzBbNh\n\tQp5WjKyJOdn6It2KeS57OWJH29J4ny2ZXGJSckJM=","Date":"Tue, 19 Nov 2024 10:17:29 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 01/17] ipa: libipa: vector: Add mutable x(), y() and\n\tz() accessors","Message-ID":"<20241119081729.GB31681@pendragon.ideasonboard.com>","References":"<20241118221618.13953-1-laurent.pinchart@ideasonboard.com>\n\t<20241118221618.13953-2-laurent.pinchart@ideasonboard.com>\n\t<5cKEjoyO3lndE_ROm1YxerxiIV766m82rHqrsU01TYl07jQlhJGgdsfhkXduWT58Aw5SHh74X_tKRAaVvSetK2GTDD3JtbTg4QS6QP7kaUY=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5cKEjoyO3lndE_ROm1YxerxiIV766m82rHqrsU01TYl07jQlhJGgdsfhkXduWT58Aw5SHh74X_tKRAaVvSetK2GTDD3JtbTg4QS6QP7kaUY=@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]