[{"id":26344,"web_url":"https://patchwork.libcamera.org/comment/26344/","msgid":"<167460538048.1334668.71671031608644644@Monstersaurus>","date":"2023-01-25T00:09:40","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> gcc-13 warns that the valueOrTuple() function has a redundant\n> std::move() in a return statement:\n> \n> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> 28 | return std::move(t);\n\nohhh - this may be just too pedantic for me. Explicitly stating\nstd::move(t) when the compiler knows it is a move may be redundant to\nthe compiler, but it's not redundant to the reader?!\n\nDoesn't this help make it clear that the t is being moved... in which\ncase it's helpful self documenting code?\n\nI'm normally all for warnings, but this one is annoying.\n\nhttps://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\nstates that this isn't a 'pessimizing' operation, it's just redundant,\nbut it does make it clearer that a move is expected to occur?\n\n> \n> Drop it.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/py/libcamera/py_helpers.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n> index 79891ab63862..5bedea047e31 100644\n> --- a/src/py/libcamera/py_helpers.cpp\n> +++ b/src/py/libcamera/py_helpers.cpp\n> @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv)\n> for (size_t i = 0; i < cv.numElements(); ++i)\n> t[i] = v[i];\n> \n> - return std::move(t);\n> + return t;\n> }\n> \n> return py::cast(cv.get());\n> \n> base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63\n> prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0\n> prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42\n> prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","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 59E92BEFBE\n\tfor ;\n\tWed, 25 Jan 2023 00:09:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0347603C7;\n\tWed, 25 Jan 2023 01:09:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C7F7603C7\n\tfor ;\n\tWed, 25 Jan 2023 01:09:43 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 000BD6D0;\n\tWed, 25 Jan 2023 01:09:42 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674605384;\n\tbh=CAFCc4sIIb+xB5ZGCT1nVbz1fErZ4RctBLlHhbrZg/c=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=daqfhMQI3sZHcutKuYSUN8+DnAAwa9lzIgkCtXZ4P+jE6BfQBJMQWI2qlqcAds9vZ\n\tipdlqJUfyFydikV4h9halOR86+m1o4UwNW7nzDjl6RDmBy/ayh3uMJosFLzkanwdU6\n\tqhT1/7x0xxIG1Acy7kxYGdP4wEMRIDF4L0TLb5j53H7w7RryR16L3T12d4pqlnXUtK\n\thQV53F51QJEZVC3qsaKMxgH3RWhc0X017wkNRKVrb+pSC4olfohMdDMCxlAV9nnvHQ\n\tynQ6cKECE6ZIZiJ0Dx7UKHGByK0qxXO/HYtC2gM4Ie4dKPZN7V5z+XkA/edGlxSmHA\n\tdZvFWunwm3wIQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674605383;\n\tbh=CAFCc4sIIb+xB5ZGCT1nVbz1fErZ4RctBLlHhbrZg/c=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=iV15q9Vq7/UYhvXKYFmCYFFSnzHpE0itPK8SBTkzkXRTPbwVChGclQTV/kphE9ghA\n\teGwZPN6yQqw73LDGRcX6uuPBCOS6JiycT6A6AGw494OiIeYBO3Pel2OGlCJBCRDxsn\n\tFGBuD/WeIdqQhfGDvR9a/gPKKkhSuxlQDLVz/t3k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iV15q9Vq\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 25 Jan 2023 00:09:40 +0000","Message-ID":"<167460538048.1334668.71671031608644644@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26346,"web_url":"https://patchwork.libcamera.org/comment/26346/","msgid":"","date":"2023-01-25T09:42:56","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> > gcc-13 warns that the valueOrTuple() function has a redundant\n> > std::move() in a return statement:\n> > \n> > ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> > ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> > ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> > 28 | return std::move(t);\n> \n> ohhh - this may be just too pedantic for me. Explicitly stating\n> std::move(t) when the compiler knows it is a move may be redundant to\n> the compiler, but it's not redundant to the reader?!\n> \n> Doesn't this help make it clear that the t is being moved... in which\n> case it's helpful self documenting code?\n> \n> I'm normally all for warnings, but this one is annoying.\n> \n> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> states that this isn't a 'pessimizing' operation, it's just redundant,\n> but it does make it clearer that a move is expected to occur?\n\nAdding more context, the function is implemented as\n\n\tif (cv.isArray()) {\n\t\tconst T *v = reinterpret_cast(cv.data().data());\n\t\tauto t = py::tuple(cv.numElements());\n\n\t\tfor (size_t i = 0; i < cv.numElements(); ++i)\n\t\t\tt[i] = v[i];\n\n\t\treturn std::move(t);\n\t}\n\n\treturn py::cast(cv.get());\n\nThe type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\nproduces the same warning), which inherits from py::object. We thus seem\nto be in the last case described by the above link:\n\n There are situations where returning std::move (expr) makes sense,\n however. The rules for the implicit move require that the selected\n constructor take an rvalue reference to the returned object's type.\n Sometimes that isn't the case. For example, when a function returns\n an object whose type is a class derived from the class type the\n function returns. In that case, overload resolution is performed a\n second time, this time treating the object as an lvalue:\n\n struct U { };\n struct T : U { };\n\n U f()\n {\n T t;\n return std::move (t);\n }\n\ng++-13 produces a warning when compiling that code:\n\nmove.cpp: In function ‘U f()’:\nmove.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n 9 | return std::move (t);\n | ~~~~~~~~~~^~~\nmove.cpp:9:26: note: remove ‘std::move’ call\n\nThis may also be a false positive of gcc ?\n\n> > Drop it.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/py/libcamera/py_helpers.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n> > index 79891ab63862..5bedea047e31 100644\n> > --- a/src/py/libcamera/py_helpers.cpp\n> > +++ b/src/py/libcamera/py_helpers.cpp\n> > @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv)\n> > for (size_t i = 0; i < cv.numElements(); ++i)\n> > t[i] = v[i];\n> > \n> > - return std::move(t);\n> > + return t;\n> > }\n> > \n> > return py::cast(cv.get());\n> > \n> > base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63\n> > prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0\n> > prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42\n> > prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391","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 B1155BEFBE\n\tfor ;\n\tWed, 25 Jan 2023 09:43:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F18D2625E4;\n\tWed, 25 Jan 2023 10:43:00 +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 C70EA61EFA\n\tfor ;\n\tWed, 25 Jan 2023 10:42:59 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D68B6E0;\n\tWed, 25 Jan 2023 10:42:59 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674639781;\n\tbh=TPFiy+mwq53m1DoKuAgaeaviCS7Tzk3YLW3UIf5ZKEA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=hF5nTJ23xgNkdMr49DBuuawuBBt3+JGwUyiE478/Z88+7zExlASiWZdsRJICifOg+\n\tGFj9i5K92HM/LXMSNHbLmCqpXbp8FmbSQ8UXoW90pWoAc8ektRLYciMmOFC8ITuC6e\n\tEswyD42pG8noB5x7qAxqrhIJXtF6nmU0BBkA08UvFwzWVv8foa6BnsJI2c2H/6GJ8h\n\t8NAbDNkuV1x6LKlkcYvsKtbToTm0OQ132HJPWFpex4zm92EGpgCky19iWBeM9VCE7s\n\tnSqvrACa3KbyTZmWG3fzQffBsijEdOX2X6T18DhBaPq+Nc7v9n4TIlbFzfKbmrMzOo\n\tfEfMXOG9Ebdqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674639779;\n\tbh=TPFiy+mwq53m1DoKuAgaeaviCS7Tzk3YLW3UIf5ZKEA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UlzLcH5QedJdsyoVUj6hg4murytDiVMgDl2AsIyE5opHbt9VYZNodwd6jPepW2X13\n\tUNh5pvR7EULQrSj4jhRZPs4ApMcbBmHDw0s6Na2Y0B74IeC3+kzhhqiaVKPbDLkTcE\n\t/xHv4Bd/4TQhgN0dfXoshcoo/ylQvd0vret4I9/w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UlzLcH5Q\"; dkim-atps=neutral","Date":"Wed, 25 Jan 2023 11:42:56 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<167460538048.1334668.71671031608644644@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26856,"web_url":"https://patchwork.libcamera.org/comment/26856/","msgid":"<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","date":"2023-04-05T06:49:45","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> Hi Kieran,\n> \n> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n>>> gcc-13 warns that the valueOrTuple() function has a redundant\n>>> std::move() in a return statement:\n>>>\n>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n>>> 28 | return std::move(t);\n>>\n>> ohhh - this may be just too pedantic for me. Explicitly stating\n>> std::move(t) when the compiler knows it is a move may be redundant to\n>> the compiler, but it's not redundant to the reader?!\n>>\n>> Doesn't this help make it clear that the t is being moved... in which\n>> case it's helpful self documenting code?\n>>\n>> I'm normally all for warnings, but this one is annoying.\n>>\n>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n>> states that this isn't a 'pessimizing' operation, it's just redundant,\n>> but it does make it clearer that a move is expected to occur?\n> \n> Adding more context, the function is implemented as\n> \n> \tif (cv.isArray()) {\n> \t\tconst T *v = reinterpret_cast(cv.data().data());\n> \t\tauto t = py::tuple(cv.numElements());\n> \n> \t\tfor (size_t i = 0; i < cv.numElements(); ++i)\n> \t\t\tt[i] = v[i];\n> \n> \t\treturn std::move(t);\n> \t}\n> \n> \treturn py::cast(cv.get());\n> \n> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> produces the same warning), which inherits from py::object. We thus seem\n> to be in the last case described by the above link:\n> \n> There are situations where returning std::move (expr) makes sense,\n> however. The rules for the implicit move require that the selected\n> constructor take an rvalue reference to the returned object's type.\n> Sometimes that isn't the case. For example, when a function returns\n> an object whose type is a class derived from the class type the\n> function returns. In that case, overload resolution is performed a\n> second time, this time treating the object as an lvalue:\n> \n> struct U { };\n> struct T : U { };\n> \n> U f()\n> {\n> T t;\n> return std::move (t);\n> }\n> \n> g++-13 produces a warning when compiling that code:\n> \n> move.cpp: In function ‘U f()’:\n> move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> 9 | return std::move (t);\n> | ~~~~~~~~~~^~~\n> move.cpp:9:26: note: remove ‘std::move’ call\n> \n> This may also be a false positive of gcc ?\n\nI don't have gcc 13, nor does godbolt.org, but other gcc nor clang \nversions don't complain.\n\nWith some testing on godbolt, with and without std::move the end result \nis the same (with -O2) on the compilers I tested.\n\nSo... I don't know. The text you pasted seems to suggest that \nstd::move() would be needed there, but I don't see a diff (then again, \nmy test code is just test code, not the actual py code we have). I'm \nfine either way, but if gcc 13 is not much used yet, maybe we should \nwait a bit?\n\nAlso, a bit beside the point, I'm actually a bit surprised that\n\nU f()\n{\n\treturn T();\n}\n\nworks without warnings (even if I add fields to U and T). It's silently \nthrowing away the T specific parts, only keeping the U parts.\n\n Tomi","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 EC5B1BD808\n\tfor ;\n\tWed, 5 Apr 2023 06:49:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44ADC6274B;\n\tWed, 5 Apr 2023 08:49:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89FDB61EC2\n\tfor ;\n\tWed, 5 Apr 2023 08:49:49 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5C98905;\n\tWed, 5 Apr 2023 08:49:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680677391;\n\tbh=8mUTFqABuAyc6PDQSHbQtDWJy986TdDZiZZujm7KKwU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=adel9nTGob2kqlBxevw6NSwANkWMi9DJhDK6kK25oWLfLzjEb6pTq43HxYrs0LPDr\n\tuK+sXRyHxx116xRXDSa1DPxrl+QjJBDKRzmipMS11clj5Y389bOhdyQzwShxG5/nG+\n\tJCgJankbtEPLayNgIbEmGOiaS9WZHMn6OCu6K0YMo0/lmX9qLmaVG4Bfzh0ShXXAZU\n\tsQlQkUjJL6x1SwCTpFEt9QWgAmGV91yP2M7Rbnpd8pUDiDMCcsWHfZ+akSxW0sORCD\n\tAMthaWNzelGjEnrW6BxJSoUiZ7Nk68jMV6A8HJ00N5cFLP9oKEt7VOp9eRHAzLsFE9\n\tQYicWGY9pKe7Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680677389;\n\tbh=8mUTFqABuAyc6PDQSHbQtDWJy986TdDZiZZujm7KKwU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vc4rHF+dGq/D1RuI5iL8M2Jk40ZSOB1k9szf86yucv+25Ke/ldHNsTAlJNxdJU6ay\n\tsjbv43PvJDspjGQl4DhDb/5I4OOov0UF4+OEYcAUKIF9KMkCFNS80jN+/IdYkOnsG8\n\t9kKKWFeZNFykH16fyn5aN0Q7Dg+F99dmKmYsuK0E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vc4rHF+d\"; dkim-atps=neutral","Message-ID":"<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","Date":"Wed, 5 Apr 2023 09:49:45 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.9.0","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26992,"web_url":"https://patchwork.libcamera.org/comment/26992/","msgid":"","date":"2023-04-28T14:43:52","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 05/04/2023 09:49, Tomi Valkeinen via libcamera-devel wrote:\n> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n>> Hi Kieran,\n>>\n>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n>>>> gcc-13 warns that the valueOrTuple() function has a redundant\n>>>> std::move() in a return statement:\n>>>>\n>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of \n>>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with \n>>>> T = bool]’:\n>>>> ../../src/py/libcamera/py_helpers.cpp:38:28:   required from here\n>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move \n>>>> in return statement [-Werror=redundant-move]\n>>>>     28 |                 return std::move(t);\n>>>\n>>> ohhh - this may be just too pedantic for me. Explicitly stating\n>>> std::move(t) when the compiler knows it is a move may be redundant to\n>>> the compiler, but it's not redundant to the reader?!\n>>>\n>>> Doesn't this help make it clear that the t is being moved... in which\n>>> case it's helpful self documenting code?\n>>>\n>>> I'm normally all for warnings, but this one is annoying.\n>>>\n>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n>>> states that this isn't a 'pessimizing' operation, it's just redundant,\n>>> but it does make it clearer that a move is expected to occur?\n>>\n>> Adding more context, the function is implemented as\n>>\n>>     if (cv.isArray()) {\n>>         const T *v = reinterpret_cast(cv.data().data());\n>>         auto t = py::tuple(cv.numElements());\n>>\n>>         for (size_t i = 0; i < cv.numElements(); ++i)\n>>             t[i] = v[i];\n>>\n>>         return std::move(t);\n>>     }\n>>\n>>     return py::cast(cv.get());\n>>\n>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n>> produces the same warning), which inherits from py::object. We thus seem\n>> to be in the last case described by the above link:\n>>\n>>      There are situations where returning std::move (expr) makes sense,\n>>      however. The rules for the implicit move require that the selected\n>>      constructor take an rvalue reference to the returned object's type.\n>>      Sometimes that isn't the case. For example, when a function returns\n>>      an object whose type is a class derived from the class type the\n>>      function returns. In that case, overload resolution is performed a\n>>      second time, this time treating the object as an lvalue:\n>>\n>>      struct U { };\n>>      struct T : U { };\n>>\n>>      U f()\n>>      {\n>>              T t;\n>>              return std::move (t);\n>>      }\n>>\n>> g++-13 produces a warning when compiling that code:\n>>\n>> move.cpp: In function ‘U f()’:\n>> move.cpp:9:26: warning: redundant move in return statement \n>> [-Wredundant-move]\n>>      9 |         return std::move (t);\n>>        |                ~~~~~~~~~~^~~\n>> move.cpp:9:26: note: remove ‘std::move’ call\n>>\n>> This may also be a false positive of gcc ?\n> \n> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> versions don't complain.\n> \n> With some testing on godbolt, with and without std::move the end result \n> is the same (with -O2) on the compilers I tested.\n> \n> So... I don't know. The text you pasted seems to suggest that \n> std::move() would be needed there, but I don't see a diff (then again, \n> my test code is just test code, not the actual py code we have). I'm \n> fine either way, but if gcc 13 is not much used yet, maybe we should \n> wait a bit?\n> \n> Also, a bit beside the point, I'm actually a bit surprised that\n> \n> U f()\n> {\n>     return T();\n> }\n> \n> works without warnings (even if I add fields to U and T). It's silently \n> throwing away the T specific parts, only keeping the U parts.\n> \n>  Tomi\n> \n\nMaybe add a comment, noting that this used to have std::move(), but it \nwas dropped as gcc 13 warns. And we're not sure if the warning is correct.\n\nReviewed-by: Tomi Valkeinen \n\n Tomi","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 C7655C0DA4\n\tfor ;\n\tFri, 28 Apr 2023 14:43:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8E2E627DF;\n\tFri, 28 Apr 2023 16:43:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5164F627D3\n\tfor ;\n\tFri, 28 Apr 2023 16:43:56 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3430D7FC;\n\tFri, 28 Apr 2023 16:43:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682693037;\n\tbh=BWC6mGE2SjYXd+28T1kLziTsQypvhcxPFpnMFxnJN6s=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZOODkNMFNH7VR70eZ2ChG4ulwoPkEk3rl1gr2QVl9Fgtwo5FLJbzx0OE39QOW95bC\n\t3NARJIxisrAaeDBKU1sDimjRy7sBNYDUufhiqoEG74i1NEMn3bY9+41TavOKfJQL0b\n\t5abR/a054cx2CM3+dwkaTNLSk/3SmQFvVYKJ9WdHZJGlzAGI/ge11TEhMr8JnwxrtM\n\tVK3N0oRHUS2xlgQE4UXwV/G/bCpmQSwCtWYlmqfxbyEnxJQnoDz5ZEig/0e55Hkp0u\n\tF+igqjlYJUR0kQGRNm+hluhqmSX75RaZFgf73Fb6IsUEkt7yo+q2aN6OBDMhHKX5wE\n\tHYUyOJvc3y6LQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682693023;\n\tbh=BWC6mGE2SjYXd+28T1kLziTsQypvhcxPFpnMFxnJN6s=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ZmLvdeTqro37pIT1S0XbZBNcR/rvgcH8dPrXPKYNX/C9JvXJulF6JOV0+DoZLKJpl\n\tO1hxSgGjPPzm6r0gbIREzKiGYbKQIAphuAY7qEn7f4LvNPBOg+kXQOwUvZj93TT0H5\n\taCvzVIHtVy7IJhIMgV5YjcqV5FA9aXvNkN0VSBgc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZmLvdeTq\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 28 Apr 2023 17:43:52 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.10.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","In-Reply-To":"<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26993,"web_url":"https://patchwork.libcamera.org/comment/26993/","msgid":"<20230428145107.GD13163@pendragon.ideasonboard.com>","date":"2023-04-28T14:51:07","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> >>> gcc-13 warns that the valueOrTuple() function has a redundant\n> >>> std::move() in a return statement:\n> >>>\n> >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> >>> 28 | return std::move(t);\n> >>\n> >> ohhh - this may be just too pedantic for me. Explicitly stating\n> >> std::move(t) when the compiler knows it is a move may be redundant to\n> >> the compiler, but it's not redundant to the reader?!\n> >>\n> >> Doesn't this help make it clear that the t is being moved... in which\n> >> case it's helpful self documenting code?\n> >>\n> >> I'm normally all for warnings, but this one is annoying.\n> >>\n> >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> >> states that this isn't a 'pessimizing' operation, it's just redundant,\n> >> but it does make it clearer that a move is expected to occur?\n> > \n> > Adding more context, the function is implemented as\n> > \n> > \tif (cv.isArray()) {\n> > \t\tconst T *v = reinterpret_cast(cv.data().data());\n> > \t\tauto t = py::tuple(cv.numElements());\n> > \n> > \t\tfor (size_t i = 0; i < cv.numElements(); ++i)\n> > \t\t\tt[i] = v[i];\n> > \n> > \t\treturn std::move(t);\n> > \t}\n> > \n> > \treturn py::cast(cv.get());\n> > \n> > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> > produces the same warning), which inherits from py::object. We thus seem\n> > to be in the last case described by the above link:\n> > \n> > There are situations where returning std::move (expr) makes sense,\n> > however. The rules for the implicit move require that the selected\n> > constructor take an rvalue reference to the returned object's type.\n> > Sometimes that isn't the case. For example, when a function returns\n> > an object whose type is a class derived from the class type the\n> > function returns. In that case, overload resolution is performed a\n> > second time, this time treating the object as an lvalue:\n> > \n> > struct U { };\n> > struct T : U { };\n> > \n> > U f()\n> > {\n> > T t;\n> > return std::move (t);\n> > }\n> > \n> > g++-13 produces a warning when compiling that code:\n> > \n> > move.cpp: In function ‘U f()’:\n> > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> > 9 | return std::move (t);\n> > | ~~~~~~~~~~^~~\n> > move.cpp:9:26: note: remove ‘std::move’ call\n> > \n> > This may also be a false positive of gcc ?\n> \n> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> versions don't complain.\n> \n> With some testing on godbolt, with and without std::move the end result \n> is the same (with -O2) on the compilers I tested.\n> \n> So... I don't know. The text you pasted seems to suggest that \n> std::move() would be needed there, but I don't see a diff (then again, \n> my test code is just test code, not the actual py code we have). I'm \n> fine either way, but if gcc 13 is not much used yet, maybe we should \n> wait a bit?\n> \n> Also, a bit beside the point, I'm actually a bit surprised that\n> \n> U f()\n> {\n> \treturn T();\n> }\n> \n> works without warnings (even if I add fields to U and T). It's silently \n> throwing away the T specific parts, only keeping the U parts.\n\nI tried this test code:\n\n--------\n#include \n#include \n\nstruct Base {\n\tBase()\n\t{\n\t\tstd::cout << \"Base::Base()\" << std::endl;\n\t}\n\n\tBase(const Base &)\n\t{\n\t\tstd::cout << \"Base::Base(const Base &)\" << std::endl;\n\t}\n\n\tBase(Base &&)\n\t{\n\t\tstd::cout << \"Base::Base(Base &&)\" << std::endl;\n\t}\n};\n\nstruct Derived : Base {\n};\n\nBase f()\n{\n\tDerived t;\n\treturn std::move(t);\n}\n\nBase g()\n{\n\tDerived t;\n\treturn t;\n}\n\nint main()\n{\n\tf();\n\tg();\n\n\treturn 0;\n}\n--------\n\n$ g++-12 -W -Wall -o move2 move2.cpp && ./move2 \nBase::Base()\nBase::Base(Base &&)\nBase::Base()\nBase::Base(const Base &)\n\n$ g++-13 -W -Wall -o move2 move2.cpp && ./move2 \nmove2.cpp: In function ‘Base f()’:\nmove2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n 27 | return std::move(t);\n | ~~~~~~~~~^~~\nmove2.cpp:27:25: note: remove ‘std::move’ call\nBase::Base()\nBase::Base(Base &&)\nBase::Base()\nBase::Base(Base &&)\n\nThis is annoying. The move seems to be redundant indeed with g++-13, but\ndropping it results in the copy constructor being used with g++-12 and\nearlier.","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 B7AA1BDCBD\n\tfor ;\n\tFri, 28 Apr 2023 14:50:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CABA627E0;\n\tFri, 28 Apr 2023 16:50:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB061627D3\n\tfor ;\n\tFri, 28 Apr 2023 16:50:57 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26D4E7FC;\n\tFri, 28 Apr 2023 16:50:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682693459;\n\tbh=ontMWqJEB000YGeT0jmqEELnnP1uCpiQEk0yGgjdvpE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=AZvFnI9DtaZa7+eu0R00LaP8E5FW+y74YSJ3WmJue0rpgIa8D6YumQRaGL3y7GGNb\n\tPY9BSgEQUP05jDg864HUKQW20wH+4uwaL1ZHVffHlrq4QeJSZmc4lfnKhflUR0fx7h\n\tL3ZVA9skFaTYRhpoZ3gL21K2oKyykvhLlXKL34nMbSLzSKKd83+dLOcl1etR2JeUMH\n\t1Cs9DFyAHZkLx6rR0IKXwhuqORTcB1SlVgFurvIn5pgqzmX0E3rOlIfnowr3tdscsl\n\tsCyCflz1uXefA1307SXQXDufEly0M+lHxhYYMu9a2jBvJJR95RETyUFWB4u2vo7XK2\n\tKqdlQtBd+iYDQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682693445;\n\tbh=ontMWqJEB000YGeT0jmqEELnnP1uCpiQEk0yGgjdvpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HTa2BmBe33w40Zf5VYB4P42xcBl8BnTp2QO0bHwCvYYMkCVUm0wyah5SHkCRcSc02\n\tpeGyX8CxLHp1ckrVuJ8rYNKZz1E+yMZnlHF6OUkCPZXcL9SBwopIkbyADi52t8Saie\n\t27mHdt2mJCAYOcnzI08DwIIKcMNIG+lZXOgJfznU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HTa2BmBe\"; dkim-atps=neutral","Date":"Fri, 28 Apr 2023 17:51:07 +0300","To":"Tomi Valkeinen ","Message-ID":"<20230428145107.GD13163@pendragon.ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26994,"web_url":"https://patchwork.libcamera.org/comment/26994/","msgid":"<4bf1bcf1-9d36-2d5c-f65d-ff156baa64d4@ideasonboard.com>","date":"2023-04-28T15:03:18","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 28/04/2023 17:51, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n>> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n>>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n>>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n>>>>> gcc-13 warns that the valueOrTuple() function has a redundant\n>>>>> std::move() in a return statement:\n>>>>>\n>>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n>>>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n>>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n>>>>> 28 | return std::move(t);\n>>>>\n>>>> ohhh - this may be just too pedantic for me. Explicitly stating\n>>>> std::move(t) when the compiler knows it is a move may be redundant to\n>>>> the compiler, but it's not redundant to the reader?!\n>>>>\n>>>> Doesn't this help make it clear that the t is being moved... in which\n>>>> case it's helpful self documenting code?\n>>>>\n>>>> I'm normally all for warnings, but this one is annoying.\n>>>>\n>>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n>>>> states that this isn't a 'pessimizing' operation, it's just redundant,\n>>>> but it does make it clearer that a move is expected to occur?\n>>>\n>>> Adding more context, the function is implemented as\n>>>\n>>> \tif (cv.isArray()) {\n>>> \t\tconst T *v = reinterpret_cast(cv.data().data());\n>>> \t\tauto t = py::tuple(cv.numElements());\n>>>\n>>> \t\tfor (size_t i = 0; i < cv.numElements(); ++i)\n>>> \t\t\tt[i] = v[i];\n>>>\n>>> \t\treturn std::move(t);\n>>> \t}\n>>>\n>>> \treturn py::cast(cv.get());\n>>>\n>>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n>>> produces the same warning), which inherits from py::object. We thus seem\n>>> to be in the last case described by the above link:\n>>>\n>>> There are situations where returning std::move (expr) makes sense,\n>>> however. The rules for the implicit move require that the selected\n>>> constructor take an rvalue reference to the returned object's type.\n>>> Sometimes that isn't the case. For example, when a function returns\n>>> an object whose type is a class derived from the class type the\n>>> function returns. In that case, overload resolution is performed a\n>>> second time, this time treating the object as an lvalue:\n>>>\n>>> struct U { };\n>>> struct T : U { };\n>>>\n>>> U f()\n>>> {\n>>> T t;\n>>> return std::move (t);\n>>> }\n>>>\n>>> g++-13 produces a warning when compiling that code:\n>>>\n>>> move.cpp: In function ‘U f()’:\n>>> move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n>>> 9 | return std::move (t);\n>>> | ~~~~~~~~~~^~~\n>>> move.cpp:9:26: note: remove ‘std::move’ call\n>>>\n>>> This may also be a false positive of gcc ?\n>>\n>> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang\n>> versions don't complain.\n>>\n>> With some testing on godbolt, with and without std::move the end result\n>> is the same (with -O2) on the compilers I tested.\n>>\n>> So... I don't know. The text you pasted seems to suggest that\n>> std::move() would be needed there, but I don't see a diff (then again,\n>> my test code is just test code, not the actual py code we have). I'm\n>> fine either way, but if gcc 13 is not much used yet, maybe we should\n>> wait a bit?\n>>\n>> Also, a bit beside the point, I'm actually a bit surprised that\n>>\n>> U f()\n>> {\n>> \treturn T();\n>> }\n>>\n>> works without warnings (even if I add fields to U and T). It's silently\n>> throwing away the T specific parts, only keeping the U parts.\n> \n> I tried this test code:\n> \n> --------\n> #include \n> #include \n> \n> struct Base {\n> \tBase()\n> \t{\n> \t\tstd::cout << \"Base::Base()\" << std::endl;\n> \t}\n> \n> \tBase(const Base &)\n> \t{\n> \t\tstd::cout << \"Base::Base(const Base &)\" << std::endl;\n> \t}\n> \n> \tBase(Base &&)\n> \t{\n> \t\tstd::cout << \"Base::Base(Base &&)\" << std::endl;\n> \t}\n> };\n> \n> struct Derived : Base {\n> };\n> \n> Base f()\n> {\n> \tDerived t;\n> \treturn std::move(t);\n> }\n> \n> Base g()\n> {\n> \tDerived t;\n> \treturn t;\n> }\n> \n> int main()\n> {\n> \tf();\n> \tg();\n> \n> \treturn 0;\n> }\n> --------\n> \n> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2\n> Base::Base()\n> Base::Base(Base &&)\n> Base::Base()\n> Base::Base(const Base &)\n> \n> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2\n> move2.cpp: In function ‘Base f()’:\n> move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n> 27 | return std::move(t);\n> | ~~~~~~~~~^~~\n> move2.cpp:27:25: note: remove ‘std::move’ call\n> Base::Base()\n> Base::Base(Base &&)\n> Base::Base()\n> Base::Base(Base &&)\n> \n> This is annoying. The move seems to be redundant indeed with g++-13, but\n> dropping it results in the copy constructor being used with g++-12 and\n> earlier.\n> \n\nWhat about:\n\n\tDerived t;\n\tBase& b = t;\n\treturn std::move(b);\n\n Tomi","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 3EEB7C0DA4\n\tfor ;\n\tFri, 28 Apr 2023 15:03:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A078627DF;\n\tFri, 28 Apr 2023 17:03:22 +0200 (CEST)","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 60D93627D3\n\tfor ;\n\tFri, 28 Apr 2023 17:03:21 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CBC927C;\n\tFri, 28 Apr 2023 17:03:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682694202;\n\tbh=EuJo9i31VKrLd4t4HhszqEuzP4CygWGR2HzeRobQksE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1qpw6TFAxkdLsCWvphPRCXzNCYb9OtxvdXzV4yQENhezz8Z2TzQvfEPapYRB8EnVt\n\tOryoSdjeh9o/99XOl8RrbStamKJMiCFGmvNmYB9uZSSl3eKiQ4u4LA8ZPjhpipjuv2\n\thSqKIch42VwX6u600bwSY0O676rfUayu5/UT4STjAYErjjFNa+sOMP0Wuxcd63ZI6G\n\tqd44qhawPuz+437cOaRq97SLHZZ25pIZAQvcZ1ANjeSKk+lpPm+KXRtakZXMy5J9eN\n\tH+lcQC5v0gcxl+6vuZ14gpKNqlWvuaYJEmP7NjczO3iG9G2b6IHwLBoXjlYfejzrCk\n\tmTB/GgQsEIqmA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682694188;\n\tbh=EuJo9i31VKrLd4t4HhszqEuzP4CygWGR2HzeRobQksE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EHfft+9ZzMFYFHvTcEKLb+w1zJBoWNAHcooPDT0leL7aNa7rn2Aq3nmhz0RgfNbS9\n\tLhd1YZzFNqV0i64+/k4rE8OeKOuh7WbLl5GdK99ctWK83PYZnlt2vpFZs265tZPuQZ\n\tYDBkKZD9lRDN6y8FvxM+5/4M/7wZnDqQb5LyohHs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EHfft+9Z\"; dkim-atps=neutral","Message-ID":"<4bf1bcf1-9d36-2d5c-f65d-ff156baa64d4@ideasonboard.com>","Date":"Fri, 28 Apr 2023 18:03:18 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.10.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>","In-Reply-To":"<20230428145107.GD13163@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26995,"web_url":"https://patchwork.libcamera.org/comment/26995/","msgid":"<20230428150808.GA2625@pendragon.ideasonboard.com>","date":"2023-04-28T15:08:08","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Tomi,\n> \n> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n> > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> > >>> gcc-13 warns that the valueOrTuple() function has a redundant\n> > >>> std::move() in a return statement:\n> > >>>\n> > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> > >>> 28 | return std::move(t);\n> > >>\n> > >> ohhh - this may be just too pedantic for me. Explicitly stating\n> > >> std::move(t) when the compiler knows it is a move may be redundant to\n> > >> the compiler, but it's not redundant to the reader?!\n> > >>\n> > >> Doesn't this help make it clear that the t is being moved... in which\n> > >> case it's helpful self documenting code?\n> > >>\n> > >> I'm normally all for warnings, but this one is annoying.\n> > >>\n> > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> > >> states that this isn't a 'pessimizing' operation, it's just redundant,\n> > >> but it does make it clearer that a move is expected to occur?\n> > > \n> > > Adding more context, the function is implemented as\n> > > \n> > > \tif (cv.isArray()) {\n> > > \t\tconst T *v = reinterpret_cast(cv.data().data());\n> > > \t\tauto t = py::tuple(cv.numElements());\n> > > \n> > > \t\tfor (size_t i = 0; i < cv.numElements(); ++i)\n> > > \t\t\tt[i] = v[i];\n> > > \n> > > \t\treturn std::move(t);\n> > > \t}\n> > > \n> > > \treturn py::cast(cv.get());\n> > > \n> > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> > > produces the same warning), which inherits from py::object. We thus seem\n> > > to be in the last case described by the above link:\n> > > \n> > > There are situations where returning std::move (expr) makes sense,\n> > > however. The rules for the implicit move require that the selected\n> > > constructor take an rvalue reference to the returned object's type.\n> > > Sometimes that isn't the case. For example, when a function returns\n> > > an object whose type is a class derived from the class type the\n> > > function returns. In that case, overload resolution is performed a\n> > > second time, this time treating the object as an lvalue:\n> > > \n> > > struct U { };\n> > > struct T : U { };\n> > > \n> > > U f()\n> > > {\n> > > T t;\n> > > return std::move (t);\n> > > }\n> > > \n> > > g++-13 produces a warning when compiling that code:\n> > > \n> > > move.cpp: In function ‘U f()’:\n> > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> > > 9 | return std::move (t);\n> > > | ~~~~~~~~~~^~~\n> > > move.cpp:9:26: note: remove ‘std::move’ call\n> > > \n> > > This may also be a false positive of gcc ?\n> > \n> > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> > versions don't complain.\n> > \n> > With some testing on godbolt, with and without std::move the end result \n> > is the same (with -O2) on the compilers I tested.\n> > \n> > So... I don't know. The text you pasted seems to suggest that \n> > std::move() would be needed there, but I don't see a diff (then again, \n> > my test code is just test code, not the actual py code we have). I'm \n> > fine either way, but if gcc 13 is not much used yet, maybe we should \n> > wait a bit?\n> > \n> > Also, a bit beside the point, I'm actually a bit surprised that\n> > \n> > U f()\n> > {\n> > \treturn T();\n> > }\n> > \n> > works without warnings (even if I add fields to U and T). It's silently \n> > throwing away the T specific parts, only keeping the U parts.\n> \n> I tried this test code:\n> \n> --------\n> #include \n> #include \n> \n> struct Base {\n> \tBase()\n> \t{\n> \t\tstd::cout << \"Base::Base()\" << std::endl;\n> \t}\n> \n> \tBase(const Base &)\n> \t{\n> \t\tstd::cout << \"Base::Base(const Base &)\" << std::endl;\n> \t}\n> \n> \tBase(Base &&)\n> \t{\n> \t\tstd::cout << \"Base::Base(Base &&)\" << std::endl;\n> \t}\n> };\n> \n> struct Derived : Base {\n> };\n> \n> Base f()\n> {\n> \tDerived t;\n> \treturn std::move(t);\n> }\n> \n> Base g()\n> {\n> \tDerived t;\n> \treturn t;\n> }\n> \n> int main()\n> {\n> \tf();\n> \tg();\n> \n> \treturn 0;\n> }\n> --------\n> \n> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 \n> Base::Base()\n> Base::Base(Base &&)\n> Base::Base()\n> Base::Base(const Base &)\n> \n> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 \n> move2.cpp: In function ‘Base f()’:\n> move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n> 27 | return std::move(t);\n> | ~~~~~~~~~^~~\n> move2.cpp:27:25: note: remove ‘std::move’ call\n> Base::Base()\n> Base::Base(Base &&)\n> Base::Base()\n> Base::Base(Base &&)\n> \n> This is annoying. The move seems to be redundant indeed with g++-13, but\n> dropping it results in the copy constructor being used with g++-12 and\n> earlier.\n\nWorse, I tried\n\n#pragma GCC diagnostic push\n#pragma GCC diagnostic ignored \"-Wredundant-move\"\n\t\t/*\n\t\t * gcc-13 calls the py::object move constructor even without a\n\t\t * std::move(), making it redundant, but earlier gcc versions\n\t\t * call the copy constructor. Keep the move and silence the\n\t\t * warning.\n\t\t */\n\t\treturn std::move(t);\n#pragma GCC diagnostic pop\n\nwhich gave me\n\n../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’:\n../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]\n #pragma GCC diagnostic ignored \"-Wredundant-move\"\n ^~~~~~~~~~~~~~~~~~\n\nwith gcc-8.5.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 2D08CBDCBD\n\tfor ;\n\tFri, 28 Apr 2023 15:08:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F6DC627E0;\n\tFri, 28 Apr 2023 17:07:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0DF6D627D3\n\tfor ;\n\tFri, 28 Apr 2023 17:07:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B7CB4FE;\n\tFri, 28 Apr 2023 17:07:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682694479;\n\tbh=2+dyNCNmXacr7wawskZo8YCDHs4Os1E1+ccgXzd+43Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=mdC6U5LNGSXUsA2bkWsvS6wmYuRZJJlCqJLItzxODyZrxpmZ8r30jRwlPBiBBSRnj\n\tWfwV6J6N4pAe2wu3yRYYFd12VQo9LXkeiPqoaTiyI4vAvcmLA9B6gtnK6ltmySvtCN\n\tKtRVY5XzAZsjaQFyYTMVP7bsOdTjg9duX/zJaXESM5WkUEz73+VmRcPWGX8eJTV9ab\n\t4hUoBs6AcBPhQC7H0Rt2J70Qu4yJrfH2mohlVCKEmHps5m4Dh89ccMJjgbB1+EIX4Y\n\thviwal3avbec6p93L77Zory3lx4yIozUEBLDLQn9BViv//t7fZrjJ8ybtzNrS9hSeQ\n\t4kxhBJuFnAb8A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682694465;\n\tbh=2+dyNCNmXacr7wawskZo8YCDHs4Os1E1+ccgXzd+43Q=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=UKzHM81U0ip8gRogCba9F8k4qbv48e43JEDa8LFrRoglcRC7B1nrksaoiTXiyLY/u\n\ta8yPnq8JMuRRjPzegFz3UjDhjLVOv6QkKvuSAI9A0WAFZoxFUf4NIk4ATTxXxi1APb\n\tG7+8oI594aTjL1q9OFli3lfA/7MfvTfZ9IrsmsRw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UKzHM81U\"; dkim-atps=neutral","Date":"Fri, 28 Apr 2023 18:08:08 +0300","To":"Tomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20230428150808.GA2625@pendragon.ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20230428145107.GD13163@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26996,"web_url":"https://patchwork.libcamera.org/comment/26996/","msgid":"","date":"2023-04-28T15:10:32","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 28/04/2023 18:03, Tomi Valkeinen wrote:\n> On 28/04/2023 17:51, Laurent Pinchart wrote:\n>> Hi Tomi,\n>>\n>> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n>>> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n>>>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n>>>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n>>>>>> gcc-13 warns that the valueOrTuple() function has a redundant\n>>>>>> std::move() in a return statement:\n>>>>>>\n>>>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of \n>>>>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) \n>>>>>> [with T = bool]’:\n>>>>>> ../../src/py/libcamera/py_helpers.cpp:38:28:   required from here\n>>>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move \n>>>>>> in return statement [-Werror=redundant-move]\n>>>>>>      28 |                 return std::move(t);\n>>>>>\n>>>>> ohhh - this may be just too pedantic for me. Explicitly stating\n>>>>> std::move(t) when the compiler knows it is a move may be redundant to\n>>>>> the compiler, but it's not redundant to the reader?!\n>>>>>\n>>>>> Doesn't this help make it clear that the t is being moved... in which\n>>>>> case it's helpful self documenting code?\n>>>>>\n>>>>> I'm normally all for warnings, but this one is annoying.\n>>>>>\n>>>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n>>>>> states that this isn't a 'pessimizing' operation, it's just redundant,\n>>>>> but it does make it clearer that a move is expected to occur?\n>>>>\n>>>> Adding more context, the function is implemented as\n>>>>\n>>>>     if (cv.isArray()) {\n>>>>         const T *v = reinterpret_cast(cv.data().data());\n>>>>         auto t = py::tuple(cv.numElements());\n>>>>\n>>>>         for (size_t i = 0; i < cv.numElements(); ++i)\n>>>>             t[i] = v[i];\n>>>>\n>>>>         return std::move(t);\n>>>>     }\n>>>>\n>>>>     return py::cast(cv.get());\n>>>>\n>>>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n>>>> produces the same warning), which inherits from py::object. We thus \n>>>> seem\n>>>> to be in the last case described by the above link:\n>>>>\n>>>>       There are situations where returning std::move (expr) makes \n>>>> sense,\n>>>>       however. The rules for the implicit move require that the \n>>>> selected\n>>>>       constructor take an rvalue reference to the returned object's \n>>>> type.\n>>>>       Sometimes that isn't the case. For example, when a function \n>>>> returns\n>>>>       an object whose type is a class derived from the class type the\n>>>>       function returns. In that case, overload resolution is \n>>>> performed a\n>>>>       second time, this time treating the object as an lvalue:\n>>>>\n>>>>       struct U { };\n>>>>       struct T : U { };\n>>>>\n>>>>       U f()\n>>>>       {\n>>>>               T t;\n>>>>               return std::move (t);\n>>>>       }\n>>>>\n>>>> g++-13 produces a warning when compiling that code:\n>>>>\n>>>> move.cpp: In function ‘U f()’:\n>>>> move.cpp:9:26: warning: redundant move in return statement \n>>>> [-Wredundant-move]\n>>>>       9 |         return std::move (t);\n>>>>         |                ~~~~~~~~~~^~~\n>>>> move.cpp:9:26: note: remove ‘std::move’ call\n>>>>\n>>>> This may also be a false positive of gcc ?\n>>>\n>>> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang\n>>> versions don't complain.\n>>>\n>>> With some testing on godbolt, with and without std::move the end result\n>>> is the same (with -O2) on the compilers I tested.\n>>>\n>>> So... I don't know. The text you pasted seems to suggest that\n>>> std::move() would be needed there, but I don't see a diff (then again,\n>>> my test code is just test code, not the actual py code we have). I'm\n>>> fine either way, but if gcc 13 is not much used yet, maybe we should\n>>> wait a bit?\n>>>\n>>> Also, a bit beside the point, I'm actually a bit surprised that\n>>>\n>>> U f()\n>>> {\n>>>     return T();\n>>> }\n>>>\n>>> works without warnings (even if I add fields to U and T). It's silently\n>>> throwing away the T specific parts, only keeping the U parts.\n>>\n>> I tried this test code:\n>>\n>> --------\n>> #include \n>> #include \n>>\n>> struct Base {\n>>     Base()\n>>     {\n>>         std::cout << \"Base::Base()\" << std::endl;\n>>     }\n>>\n>>     Base(const Base &)\n>>     {\n>>         std::cout << \"Base::Base(const Base &)\" << std::endl;\n>>     }\n>>\n>>     Base(Base &&)\n>>     {\n>>         std::cout << \"Base::Base(Base &&)\" << std::endl;\n>>     }\n>> };\n>>\n>> struct Derived : Base {\n>> };\n>>\n>> Base f()\n>> {\n>>     Derived t;\n>>     return std::move(t);\n>> }\n>>\n>> Base g()\n>> {\n>>     Derived t;\n>>     return t;\n>> }\n>>\n>> int main()\n>> {\n>>     f();\n>>     g();\n>>\n>>     return 0;\n>> }\n>> --------\n>>\n>> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2\n>> Base::Base()\n>> Base::Base(Base &&)\n>> Base::Base()\n>> Base::Base(const Base &)\n>>\n>> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2\n>> move2.cpp: In function ‘Base f()’:\n>> move2.cpp:27:25: warning: redundant move in return statement \n>> [-Wredundant-move]\n>>     27 |         return std::move(t);\n>>        |                ~~~~~~~~~^~~\n>> move2.cpp:27:25: note: remove ‘std::move’ call\n>> Base::Base()\n>> Base::Base(Base &&)\n>> Base::Base()\n>> Base::Base(Base &&)\n>>\n>> This is annoying. The move seems to be redundant indeed with g++-13, but\n>> dropping it results in the copy constructor being used with g++-12 and\n>> earlier.\n>>\n> \n> What about:\n> \n>     Derived t;\n>     Base& b = t;\n>     return std::move(b);\n> \n\nAh, nice, godbolt now has gcc 13.\n\nThe above seems to compile fine on gcc 13, and doesn't seem to change \nthe produced code on gcc 12 and gcc 13.\n\n Tomi","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 E302CBDCBD\n\tfor ;\n\tFri, 28 Apr 2023 15:10:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FA86627E0;\n\tFri, 28 Apr 2023 17:10:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77D1F627D3\n\tfor ;\n\tFri, 28 Apr 2023 17:10:35 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 595754FE;\n\tFri, 28 Apr 2023 17:10:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682694637;\n\tbh=v3xLk66hndd4sm7rttO3bny8S7SiQyyXz7+PL1dByng=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=D0Ev9uP8qedsGzMDJLGURDMTNcIJI6F31wdKND8IrHe3wX1fi3jWVvPXNosAW7VGD\n\t+t+SMFkvGTI1Xqjfh117MXnM6TPSpOidH8okD1c0ck7GHPHvvvCwPg+iwWDV7jq+0D\n\tOmuiaghmZfmjw7Y92f6wOMbsXRPBECgK8Dp1HGJ8/fif1swy0k7WlU4/gH9E09SNKH\n\tS/Qb1ulzuG9p1lyn8k3QAp9PrSoakNwsv9UUvU7kPjT/5N7AdNQg1TrqL18H1+bnNr\n\tusESoUYKQUWdsgScxig+qBysqQT5iFRqMIH9AXTfosF1pawTHJDDqfo5JQYsjEnC5q\n\tnWPB5tQk4miTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682694622;\n\tbh=v3xLk66hndd4sm7rttO3bny8S7SiQyyXz7+PL1dByng=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=mSHUURTdspKManYzlZWdxJnimJLSCHrcCjo+TJlL+LzTI/YpxZw5V1VXLjq6IJ6gQ\n\tHOulWSeHmzBvhojGbXRSyipe74sYb/MqJZsdt/jOKmo8LGpuzJmFPuLAmFUxACbB1P\n\tIH7SIQQl45Z60Rv3dEAOxStcBUKZfdqH1T2R/hbQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mSHUURTd\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 28 Apr 2023 18:10:32 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.10.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>\n\t<4bf1bcf1-9d36-2d5c-f65d-ff156baa64d4@ideasonboard.com>","In-Reply-To":"<4bf1bcf1-9d36-2d5c-f65d-ff156baa64d4@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26997,"web_url":"https://patchwork.libcamera.org/comment/26997/","msgid":"","date":"2023-04-28T15:27:29","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 28/04/2023 18:10, Tomi Valkeinen wrote:\n> On 28/04/2023 18:03, Tomi Valkeinen wrote:\n>> On 28/04/2023 17:51, Laurent Pinchart wrote:\n>>> Hi Tomi,\n>>>\n>>> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n>>>> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n>>>>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n>>>>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n>>>>>>> gcc-13 warns that the valueOrTuple() function has a redundant\n>>>>>>> std::move() in a return statement:\n>>>>>>>\n>>>>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of \n>>>>>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) \n>>>>>>> [with T = bool]’:\n>>>>>>> ../../src/py/libcamera/py_helpers.cpp:38:28:   required from here\n>>>>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant \n>>>>>>> move in return statement [-Werror=redundant-move]\n>>>>>>>      28 |                 return std::move(t);\n>>>>>>\n>>>>>> ohhh - this may be just too pedantic for me. Explicitly stating\n>>>>>> std::move(t) when the compiler knows it is a move may be redundant to\n>>>>>> the compiler, but it's not redundant to the reader?!\n>>>>>>\n>>>>>> Doesn't this help make it clear that the t is being moved... in which\n>>>>>> case it's helpful self documenting code?\n>>>>>>\n>>>>>> I'm normally all for warnings, but this one is annoying.\n>>>>>>\n>>>>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n>>>>>> states that this isn't a 'pessimizing' operation, it's just \n>>>>>> redundant,\n>>>>>> but it does make it clearer that a move is expected to occur?\n>>>>>\n>>>>> Adding more context, the function is implemented as\n>>>>>\n>>>>>     if (cv.isArray()) {\n>>>>>         const T *v = reinterpret_cast(cv.data().data());\n>>>>>         auto t = py::tuple(cv.numElements());\n>>>>>\n>>>>>         for (size_t i = 0; i < cv.numElements(); ++i)\n>>>>>             t[i] = v[i];\n>>>>>\n>>>>>         return std::move(t);\n>>>>>     }\n>>>>>\n>>>>>     return py::cast(cv.get());\n>>>>>\n>>>>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n>>>>> produces the same warning), which inherits from py::object. We thus \n>>>>> seem\n>>>>> to be in the last case described by the above link:\n>>>>>\n>>>>>       There are situations where returning std::move (expr) makes \n>>>>> sense,\n>>>>>       however. The rules for the implicit move require that the \n>>>>> selected\n>>>>>       constructor take an rvalue reference to the returned object's \n>>>>> type.\n>>>>>       Sometimes that isn't the case. For example, when a function \n>>>>> returns\n>>>>>       an object whose type is a class derived from the class type the\n>>>>>       function returns. In that case, overload resolution is \n>>>>> performed a\n>>>>>       second time, this time treating the object as an lvalue:\n>>>>>\n>>>>>       struct U { };\n>>>>>       struct T : U { };\n>>>>>\n>>>>>       U f()\n>>>>>       {\n>>>>>               T t;\n>>>>>               return std::move (t);\n>>>>>       }\n>>>>>\n>>>>> g++-13 produces a warning when compiling that code:\n>>>>>\n>>>>> move.cpp: In function ‘U f()’:\n>>>>> move.cpp:9:26: warning: redundant move in return statement \n>>>>> [-Wredundant-move]\n>>>>>       9 |         return std::move (t);\n>>>>>         |                ~~~~~~~~~~^~~\n>>>>> move.cpp:9:26: note: remove ‘std::move’ call\n>>>>>\n>>>>> This may also be a false positive of gcc ?\n>>>>\n>>>> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang\n>>>> versions don't complain.\n>>>>\n>>>> With some testing on godbolt, with and without std::move the end result\n>>>> is the same (with -O2) on the compilers I tested.\n>>>>\n>>>> So... I don't know. The text you pasted seems to suggest that\n>>>> std::move() would be needed there, but I don't see a diff (then again,\n>>>> my test code is just test code, not the actual py code we have). I'm\n>>>> fine either way, but if gcc 13 is not much used yet, maybe we should\n>>>> wait a bit?\n>>>>\n>>>> Also, a bit beside the point, I'm actually a bit surprised that\n>>>>\n>>>> U f()\n>>>> {\n>>>>     return T();\n>>>> }\n>>>>\n>>>> works without warnings (even if I add fields to U and T). It's silently\n>>>> throwing away the T specific parts, only keeping the U parts.\n>>>\n>>> I tried this test code:\n>>>\n>>> --------\n>>> #include \n>>> #include \n>>>\n>>> struct Base {\n>>>     Base()\n>>>     {\n>>>         std::cout << \"Base::Base()\" << std::endl;\n>>>     }\n>>>\n>>>     Base(const Base &)\n>>>     {\n>>>         std::cout << \"Base::Base(const Base &)\" << std::endl;\n>>>     }\n>>>\n>>>     Base(Base &&)\n>>>     {\n>>>         std::cout << \"Base::Base(Base &&)\" << std::endl;\n>>>     }\n>>> };\n>>>\n>>> struct Derived : Base {\n>>> };\n>>>\n>>> Base f()\n>>> {\n>>>     Derived t;\n>>>     return std::move(t);\n>>> }\n>>>\n>>> Base g()\n>>> {\n>>>     Derived t;\n>>>     return t;\n>>> }\n>>>\n>>> int main()\n>>> {\n>>>     f();\n>>>     g();\n>>>\n>>>     return 0;\n>>> }\n>>> --------\n>>>\n>>> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2\n>>> Base::Base()\n>>> Base::Base(Base &&)\n>>> Base::Base()\n>>> Base::Base(const Base &)\n>>>\n>>> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2\n>>> move2.cpp: In function ‘Base f()’:\n>>> move2.cpp:27:25: warning: redundant move in return statement \n>>> [-Wredundant-move]\n>>>     27 |         return std::move(t);\n>>>        |                ~~~~~~~~~^~~\n>>> move2.cpp:27:25: note: remove ‘std::move’ call\n>>> Base::Base()\n>>> Base::Base(Base &&)\n>>> Base::Base()\n>>> Base::Base(Base &&)\n>>>\n>>> This is annoying. The move seems to be redundant indeed with g++-13, but\n>>> dropping it results in the copy constructor being used with g++-12 and\n>>> earlier.\n>>>\n>>\n>> What about:\n>>\n>>      Derived t;\n>>      Base& b = t;\n>>      return std::move(b);\n>>\n> \n> Ah, nice, godbolt now has gcc 13.\n> \n> The above seems to compile fine on gcc 13, and doesn't seem to change \n> the produced code on gcc 12 and gcc 13.\n> \n>  Tomi\n> \n\nOr this also seems to do the trick:\n\nreturn static_cast(std::move(t));\n\n Tomi","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 1C543C0DA4\n\tfor ;\n\tFri, 28 Apr 2023 15:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70E98627DF;\n\tFri, 28 Apr 2023 17:27:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 948B7627D3\n\tfor ;\n\tFri, 28 Apr 2023 17:27:33 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 786E54FE;\n\tFri, 28 Apr 2023 17:27:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682695655;\n\tbh=KVZzKTYncX8XeZC7niLlPmq0BXQnEho444dufVlyP0g=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=WxDCDGpeckwLJ2BzzOYD1tC69abEr9KXJIGLFYM060c5CfSbvcD+SizCqbHPkwl+C\n\tEx9DIRVrqGsSw7o/0R4ZJ97YJNrS3okw8KqPGD/d083Um0qM4kFdX/AocISm5QP7A+\n\t4pDca2EJtQutYRRomqw0PfYP0OC/ilggVbBoUrANMllz9EJqJr0REFq7KL1NL9Nv4Y\n\t56ImrCnfpUDfYluGIeimL8AhECZ65sbkt3gUx+Jzg64a90GNJtZAelF6PfHwBozXSV\n\tGInMOd9ZWd+yNrxKteH+OR26RL4Ag5Yzn/KyYGWv/TNyHAEhk/w/8ZXoMM4Chr5imV\n\t6V4BUTGUTpSJw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682695640;\n\tbh=KVZzKTYncX8XeZC7niLlPmq0BXQnEho444dufVlyP0g=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=qWtrV9EkIpHfLIh7+ghDAFf2z5kvB5X11QhmYxt+E9qSuuNsKSgXjxrmFderX60D7\n\t06BcIEOB4GbeZgBs0RkzdctBi4ZFardAhNSO7B6eRiPbNAz99uEX786hO5y5iqgcdf\n\tWiObmA18yN/FE2yEYftbZJV0FyP6nduNsuX7jjlY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qWtrV9Ek\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 28 Apr 2023 18:27:29 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.10.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>\n\t<4bf1bcf1-9d36-2d5c-f65d-ff156baa64d4@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26998,"web_url":"https://patchwork.libcamera.org/comment/26998/","msgid":"<168274614085.3274474.10017313386842341084@Monstersaurus>","date":"2023-04-29T05:29:00","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08)\n> On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Hi Tomi,\n> > \n> > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n> > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> > > >>> gcc-13 warns that the valueOrTuple() function has a redundant\n> > > >>> std::move() in a return statement:\n> > > >>>\n> > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> > > >>> 28 | return std::move(t);\n> > > >>\n> > > >> ohhh - this may be just too pedantic for me. Explicitly stating\n> > > >> std::move(t) when the compiler knows it is a move may be redundant to\n> > > >> the compiler, but it's not redundant to the reader?!\n> > > >>\n> > > >> Doesn't this help make it clear that the t is being moved... in which\n> > > >> case it's helpful self documenting code?\n> > > >>\n> > > >> I'm normally all for warnings, but this one is annoying.\n> > > >>\n> > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> > > >> states that this isn't a 'pessimizing' operation, it's just redundant,\n> > > >> but it does make it clearer that a move is expected to occur?\n> > > > \n> > > > Adding more context, the function is implemented as\n> > > > \n> > > > if (cv.isArray()) {\n> > > > const T *v = reinterpret_cast(cv.data().data());\n> > > > auto t = py::tuple(cv.numElements());\n> > > > \n> > > > for (size_t i = 0; i < cv.numElements(); ++i)\n> > > > t[i] = v[i];\n> > > > \n> > > > return std::move(t);\n> > > > }\n> > > > \n> > > > return py::cast(cv.get());\n> > > > \n> > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> > > > produces the same warning), which inherits from py::object. We thus seem\n> > > > to be in the last case described by the above link:\n> > > > \n> > > > There are situations where returning std::move (expr) makes sense,\n> > > > however. The rules for the implicit move require that the selected\n> > > > constructor take an rvalue reference to the returned object's type.\n> > > > Sometimes that isn't the case. For example, when a function returns\n> > > > an object whose type is a class derived from the class type the\n> > > > function returns. In that case, overload resolution is performed a\n> > > > second time, this time treating the object as an lvalue:\n> > > > \n> > > > struct U { };\n> > > > struct T : U { };\n> > > > \n> > > > U f()\n> > > > {\n> > > > T t;\n> > > > return std::move (t);\n> > > > }\n> > > > \n> > > > g++-13 produces a warning when compiling that code:\n> > > > \n> > > > move.cpp: In function ‘U f()’:\n> > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> > > > 9 | return std::move (t);\n> > > > | ~~~~~~~~~~^~~\n> > > > move.cpp:9:26: note: remove ‘std::move’ call\n> > > > \n> > > > This may also be a false positive of gcc ?\n> > > \n> > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> > > versions don't complain.\n> > > \n> > > With some testing on godbolt, with and without std::move the end result \n> > > is the same (with -O2) on the compilers I tested.\n> > > \n> > > So... I don't know. The text you pasted seems to suggest that \n> > > std::move() would be needed there, but I don't see a diff (then again, \n> > > my test code is just test code, not the actual py code we have). I'm \n> > > fine either way, but if gcc 13 is not much used yet, maybe we should \n> > > wait a bit?\n> > > \n> > > Also, a bit beside the point, I'm actually a bit surprised that\n> > > \n> > > U f()\n> > > {\n> > > return T();\n> > > }\n> > > \n> > > works without warnings (even if I add fields to U and T). It's silently \n> > > throwing away the T specific parts, only keeping the U parts.\n> > \n> > I tried this test code:\n> > \n> > --------\n> > #include \n> > #include \n> > \n> > struct Base {\n> > Base()\n> > {\n> > std::cout << \"Base::Base()\" << std::endl;\n> > }\n> > \n> > Base(const Base &)\n> > {\n> > std::cout << \"Base::Base(const Base &)\" << std::endl;\n> > }\n> > \n> > Base(Base &&)\n> > {\n> > std::cout << \"Base::Base(Base &&)\" << std::endl;\n> > }\n> > };\n> > \n> > struct Derived : Base {\n> > };\n> > \n> > Base f()\n> > {\n> > Derived t;\n> > return std::move(t);\n> > }\n> > \n> > Base g()\n> > {\n> > Derived t;\n> > return t;\n> > }\n> > \n> > int main()\n> > {\n> > f();\n> > g();\n> > \n> > return 0;\n> > }\n> > --------\n> > \n> > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 \n> > Base::Base()\n> > Base::Base(Base &&)\n> > Base::Base()\n> > Base::Base(const Base &)\n> > \n> > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 \n> > move2.cpp: In function ‘Base f()’:\n> > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n> > 27 | return std::move(t);\n> > | ~~~~~~~~~^~~\n> > move2.cpp:27:25: note: remove ‘std::move’ call\n> > Base::Base()\n> > Base::Base(Base &&)\n> > Base::Base()\n> > Base::Base(Base &&)\n> > \n> > This is annoying. The move seems to be redundant indeed with g++-13, but\n> > dropping it results in the copy constructor being used with g++-12 and\n> > earlier.\n> \n> Worse, I tried\n> \n> #pragma GCC diagnostic push\n> #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> /*\n> * gcc-13 calls the py::object move constructor even without a\n> * std::move(), making it redundant, but earlier gcc versions\n> * call the copy constructor. Keep the move and silence the\n> * warning.\n> */\n> return std::move(t);\n> #pragma GCC diagnostic pop\n> \n> which gave me\n> \n> ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’:\n> ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]\n> #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> ^~~~~~~~~~~~~~~~~~\n> \n\nGiven the pains here, can we just add -Wno-redundant-move or such for\n8.5+ ?\n--\nKieran\n\n> with gcc-8.5.0.\n> \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 68255BDCBD\n\tfor ;\n\tSat, 29 Apr 2023 05:29:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE73F627DF;\n\tSat, 29 Apr 2023 07:29:05 +0200 (CEST)","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 8494E61EAD\n\tfor ;\n\tSat, 29 Apr 2023 07:29:03 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 275EF4FE;\n\tSat, 29 Apr 2023 07:28:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682746145;\n\tbh=sFm0AEw+J22yGJvEOoDynz4dFi8wGEGv8IJOe8HwYI0=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=ekvzm/5BTfAhBPyLcureLA+30HY21A2c4USc4mPfXLDb0Qdir9aVdqgfnyO5GIOWy\n\tPlXyMcmwNt8hZMu4rhrr+q+vDLVZMfucXwIqlHADVJjhiso0dRwfcBeMdtt6M2yGIB\n\tLZLzkN77AD5E7RaOq3vOH8YehizHKT8Vw6Jf25IOpsVPCXiD9AiyOH5bo4zecywV+I\n\tsdDnJtUKEewgp6uPf8YM8KOLo4zMZBtWBq9fjQmbN2wm17OELTqXMkGwawLLyKu6t6\n\tGWkfnLaGVSnXO61yOPu8mLqJp0EtB0qKUjkFwjUj26/y4ZWrUJMKoTzvHwKyc+r8Wx\n\tn5h9X5nlOjWaA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682746130;\n\tbh=sFm0AEw+J22yGJvEOoDynz4dFi8wGEGv8IJOe8HwYI0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=e0ti+/+13aYhiqQcqgRXohuukZom4BZmft37Rqu2tIpfw90qRyVo+VNZdsi288MzX\n\tJcdVBaY8weaoJw/M2BrfVOA1FTaGSXLbul0dkPRLXpGVOKJBXd8wIVoWbvKVX2ppGw\n\tRemRRP09bWfLyhkBhgbqlFDCMk+etL9vAemqlNy0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e0ti+/+1\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230428150808.GA2625@pendragon.ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>\n\t<20230428150808.GA2625@pendragon.ideasonboard.com>","To":"Laurent Pinchart ,\n\tTomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 29 Apr 2023 06:29:00 +0100","Message-ID":"<168274614085.3274474.10017313386842341084@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26999,"web_url":"https://patchwork.libcamera.org/comment/26999/","msgid":"<20230429160321.GA19700@pendragon.ideasonboard.com>","date":"2023-04-29T16:03:21","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Sat, Apr 29, 2023 at 06:29:00AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08)\n> > On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n> > > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> > > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> > > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> > > > >>> gcc-13 warns that the valueOrTuple() function has a redundant\n> > > > >>> std::move() in a return statement:\n> > > > >>>\n> > > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> > > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> > > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> > > > >>> 28 | return std::move(t);\n> > > > >>\n> > > > >> ohhh - this may be just too pedantic for me. Explicitly stating\n> > > > >> std::move(t) when the compiler knows it is a move may be redundant to\n> > > > >> the compiler, but it's not redundant to the reader?!\n> > > > >>\n> > > > >> Doesn't this help make it clear that the t is being moved... in which\n> > > > >> case it's helpful self documenting code?\n> > > > >>\n> > > > >> I'm normally all for warnings, but this one is annoying.\n> > > > >>\n> > > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> > > > >> states that this isn't a 'pessimizing' operation, it's just redundant,\n> > > > >> but it does make it clearer that a move is expected to occur?\n> > > > > \n> > > > > Adding more context, the function is implemented as\n> > > > > \n> > > > > if (cv.isArray()) {\n> > > > > const T *v = reinterpret_cast(cv.data().data());\n> > > > > auto t = py::tuple(cv.numElements());\n> > > > > \n> > > > > for (size_t i = 0; i < cv.numElements(); ++i)\n> > > > > t[i] = v[i];\n> > > > > \n> > > > > return std::move(t);\n> > > > > }\n> > > > > \n> > > > > return py::cast(cv.get());\n> > > > > \n> > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> > > > > produces the same warning), which inherits from py::object. We thus seem\n> > > > > to be in the last case described by the above link:\n> > > > > \n> > > > > There are situations where returning std::move (expr) makes sense,\n> > > > > however. The rules for the implicit move require that the selected\n> > > > > constructor take an rvalue reference to the returned object's type.\n> > > > > Sometimes that isn't the case. For example, when a function returns\n> > > > > an object whose type is a class derived from the class type the\n> > > > > function returns. In that case, overload resolution is performed a\n> > > > > second time, this time treating the object as an lvalue:\n> > > > > \n> > > > > struct U { };\n> > > > > struct T : U { };\n> > > > > \n> > > > > U f()\n> > > > > {\n> > > > > T t;\n> > > > > return std::move (t);\n> > > > > }\n> > > > > \n> > > > > g++-13 produces a warning when compiling that code:\n> > > > > \n> > > > > move.cpp: In function ‘U f()’:\n> > > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> > > > > 9 | return std::move (t);\n> > > > > | ~~~~~~~~~~^~~\n> > > > > move.cpp:9:26: note: remove ‘std::move’ call\n> > > > > \n> > > > > This may also be a false positive of gcc ?\n> > > > \n> > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> > > > versions don't complain.\n> > > > \n> > > > With some testing on godbolt, with and without std::move the end result \n> > > > is the same (with -O2) on the compilers I tested.\n> > > > \n> > > > So... I don't know. The text you pasted seems to suggest that \n> > > > std::move() would be needed there, but I don't see a diff (then again, \n> > > > my test code is just test code, not the actual py code we have). I'm \n> > > > fine either way, but if gcc 13 is not much used yet, maybe we should \n> > > > wait a bit?\n> > > > \n> > > > Also, a bit beside the point, I'm actually a bit surprised that\n> > > > \n> > > > U f()\n> > > > {\n> > > > return T();\n> > > > }\n> > > > \n> > > > works without warnings (even if I add fields to U and T). It's silently \n> > > > throwing away the T specific parts, only keeping the U parts.\n> > > \n> > > I tried this test code:\n> > > \n> > > --------\n> > > #include \n> > > #include \n> > > \n> > > struct Base {\n> > > Base()\n> > > {\n> > > std::cout << \"Base::Base()\" << std::endl;\n> > > }\n> > > \n> > > Base(const Base &)\n> > > {\n> > > std::cout << \"Base::Base(const Base &)\" << std::endl;\n> > > }\n> > > \n> > > Base(Base &&)\n> > > {\n> > > std::cout << \"Base::Base(Base &&)\" << std::endl;\n> > > }\n> > > };\n> > > \n> > > struct Derived : Base {\n> > > };\n> > > \n> > > Base f()\n> > > {\n> > > Derived t;\n> > > return std::move(t);\n> > > }\n> > > \n> > > Base g()\n> > > {\n> > > Derived t;\n> > > return t;\n> > > }\n> > > \n> > > int main()\n> > > {\n> > > f();\n> > > g();\n> > > \n> > > return 0;\n> > > }\n> > > --------\n> > > \n> > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 \n> > > Base::Base()\n> > > Base::Base(Base &&)\n> > > Base::Base()\n> > > Base::Base(const Base &)\n> > > \n> > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 \n> > > move2.cpp: In function ‘Base f()’:\n> > > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n> > > 27 | return std::move(t);\n> > > | ~~~~~~~~~^~~\n> > > move2.cpp:27:25: note: remove ‘std::move’ call\n> > > Base::Base()\n> > > Base::Base(Base &&)\n> > > Base::Base()\n> > > Base::Base(Base &&)\n> > > \n> > > This is annoying. The move seems to be redundant indeed with g++-13, but\n> > > dropping it results in the copy constructor being used with g++-12 and\n> > > earlier.\n> > \n> > Worse, I tried\n> > \n> > #pragma GCC diagnostic push\n> > #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> > /*\n> > * gcc-13 calls the py::object move constructor even without a\n> > * std::move(), making it redundant, but earlier gcc versions\n> > * call the copy constructor. Keep the move and silence the\n> > * warning.\n> > */\n> > return std::move(t);\n> > #pragma GCC diagnostic pop\n> > \n> > which gave me\n> > \n> > ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’:\n> > ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]\n> > #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> > ^~~~~~~~~~~~~~~~~~\n> \n> Given the pains here, can we just add -Wno-redundant-move or such for\n> 8.5+ ?\n\nI think it got introduced in 9.1, and I think we can disable it, yes. As\nfar I understand the redundant moves don't cause any adverse effect.\n\n> > with gcc-8.5.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 F0BEDC0DA4\n\tfor ;\n\tSat, 29 Apr 2023 16:03:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59DC2627DF;\n\tSat, 29 Apr 2023 18:03:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C692F627D1\n\tfor ;\n\tSat, 29 Apr 2023 18:03:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5EB6175B;\n\tSat, 29 Apr 2023 18:02:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682784193;\n\tbh=OW49rJQJl0hagtMIFkwopnJ5nVujRxe59NF9bg70Ahw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HWcnzCbqr5deBp43NSvd1EOY6ZYcds2Qq4Lyj+Q3MchoxOV27gT6IUZB7c2Eu4wDX\n\tdNvtZThPGVs2f8U8LOO2PtE17VmBjhqg3M3lmLRqDhuu9E9UtR42WPtMn1/9cWT532\n\tcv6RpC7JNMnlxOM7C8/VPSweH//4BFoTaZIKwWjo2IB7QQB/RGQuntL1mX1jF8esyS\n\tr++WPamJZtIELBqJWl6l8hM0z67bZZfXurGuc9M6rqnWdzUd1Bu3I1jUoKC5fUMHtT\n\t8gSjsKB8DFuc2IeJsJww17lA2kgcnSNxUV0oj+f5zvVhoqt04TvBQIlIc34xgVcIpr\n\tykTc13bCsXuBA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682784178;\n\tbh=OW49rJQJl0hagtMIFkwopnJ5nVujRxe59NF9bg70Ahw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wWP727n/1DYRyQJPZ0C/5oFd2nTdPDjd1WdDlmpxDr6ww7scTpzvayIHD58WtnSxJ\n\tuhsn9VsolX3O4Zl5EdGip/fYyTEPLjfGiwqPHcI1HKkSvUr05mtPYFdIYqSuwYUTf4\n\tFn4HxVWYKNP6AE1TwGDePIhlNsFtMdUed+c0Z2CA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wWP727n/\"; dkim-atps=neutral","Date":"Sat, 29 Apr 2023 19:03:21 +0300","To":"Kieran Bingham ","Message-ID":"<20230429160321.GA19700@pendragon.ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>\n\t<20230428150808.GA2625@pendragon.ideasonboard.com>\n\t<168274614085.3274474.10017313386842341084@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<168274614085.3274474.10017313386842341084@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27001,"web_url":"https://patchwork.libcamera.org/comment/27001/","msgid":"<20230501031659.GB19700@pendragon.ideasonboard.com>","date":"2023-05-01T03:16:59","subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sat, Apr 29, 2023 at 07:03:21PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Sat, Apr 29, 2023 at 06:29:00AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08)\n> > > On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote:\n> > > > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote:\n> > > > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote:\n> > > > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24)\n> > > > > >>> gcc-13 warns that the valueOrTuple() function has a redundant\n> > > > > >>> std::move() in a return statement:\n> > > > > >>>\n> > > > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’:\n> > > > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here\n> > > > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move]\n> > > > > >>> 28 | return std::move(t);\n> > > > > >>\n> > > > > >> ohhh - this may be just too pedantic for me. Explicitly stating\n> > > > > >> std::move(t) when the compiler knows it is a move may be redundant to\n> > > > > >> the compiler, but it's not redundant to the reader?!\n> > > > > >>\n> > > > > >> Doesn't this help make it clear that the t is being moved... in which\n> > > > > >> case it's helpful self documenting code?\n> > > > > >>\n> > > > > >> I'm normally all for warnings, but this one is annoying.\n> > > > > >>\n> > > > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c\n> > > > > >> states that this isn't a 'pessimizing' operation, it's just redundant,\n> > > > > >> but it does make it clearer that a move is expected to occur?\n> > > > > > \n> > > > > > Adding more context, the function is implemented as\n> > > > > > \n> > > > > > if (cv.isArray()) {\n> > > > > > const T *v = reinterpret_cast(cv.data().data());\n> > > > > > auto t = py::tuple(cv.numElements());\n> > > > > > \n> > > > > > for (size_t i = 0; i < cv.numElements(); ++i)\n> > > > > > t[i] = v[i];\n> > > > > > \n> > > > > > return std::move(t);\n> > > > > > }\n> > > > > > \n> > > > > > return py::cast(cv.get());\n> > > > > > \n> > > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still\n> > > > > > produces the same warning), which inherits from py::object. We thus seem\n> > > > > > to be in the last case described by the above link:\n> > > > > > \n> > > > > > There are situations where returning std::move (expr) makes sense,\n> > > > > > however. The rules for the implicit move require that the selected\n> > > > > > constructor take an rvalue reference to the returned object's type.\n> > > > > > Sometimes that isn't the case. For example, when a function returns\n> > > > > > an object whose type is a class derived from the class type the\n> > > > > > function returns. In that case, overload resolution is performed a\n> > > > > > second time, this time treating the object as an lvalue:\n> > > > > > \n> > > > > > struct U { };\n> > > > > > struct T : U { };\n> > > > > > \n> > > > > > U f()\n> > > > > > {\n> > > > > > T t;\n> > > > > > return std::move (t);\n> > > > > > }\n> > > > > > \n> > > > > > g++-13 produces a warning when compiling that code:\n> > > > > > \n> > > > > > move.cpp: In function ‘U f()’:\n> > > > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move]\n> > > > > > 9 | return std::move (t);\n> > > > > > | ~~~~~~~~~~^~~\n> > > > > > move.cpp:9:26: note: remove ‘std::move’ call\n> > > > > > \n> > > > > > This may also be a false positive of gcc ?\n> > > > > \n> > > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang \n> > > > > versions don't complain.\n> > > > > \n> > > > > With some testing on godbolt, with and without std::move the end result \n> > > > > is the same (with -O2) on the compilers I tested.\n> > > > > \n> > > > > So... I don't know. The text you pasted seems to suggest that \n> > > > > std::move() would be needed there, but I don't see a diff (then again, \n> > > > > my test code is just test code, not the actual py code we have). I'm \n> > > > > fine either way, but if gcc 13 is not much used yet, maybe we should \n> > > > > wait a bit?\n> > > > > \n> > > > > Also, a bit beside the point, I'm actually a bit surprised that\n> > > > > \n> > > > > U f()\n> > > > > {\n> > > > > return T();\n> > > > > }\n> > > > > \n> > > > > works without warnings (even if I add fields to U and T). It's silently \n> > > > > throwing away the T specific parts, only keeping the U parts.\n> > > > \n> > > > I tried this test code:\n> > > > \n> > > > --------\n> > > > #include \n> > > > #include \n> > > > \n> > > > struct Base {\n> > > > Base()\n> > > > {\n> > > > std::cout << \"Base::Base()\" << std::endl;\n> > > > }\n> > > > \n> > > > Base(const Base &)\n> > > > {\n> > > > std::cout << \"Base::Base(const Base &)\" << std::endl;\n> > > > }\n> > > > \n> > > > Base(Base &&)\n> > > > {\n> > > > std::cout << \"Base::Base(Base &&)\" << std::endl;\n> > > > }\n> > > > };\n> > > > \n> > > > struct Derived : Base {\n> > > > };\n> > > > \n> > > > Base f()\n> > > > {\n> > > > Derived t;\n> > > > return std::move(t);\n> > > > }\n> > > > \n> > > > Base g()\n> > > > {\n> > > > Derived t;\n> > > > return t;\n> > > > }\n> > > > \n> > > > int main()\n> > > > {\n> > > > f();\n> > > > g();\n> > > > \n> > > > return 0;\n> > > > }\n> > > > --------\n> > > > \n> > > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 \n> > > > Base::Base()\n> > > > Base::Base(Base &&)\n> > > > Base::Base()\n> > > > Base::Base(const Base &)\n> > > > \n> > > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 \n> > > > move2.cpp: In function ‘Base f()’:\n> > > > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move]\n> > > > 27 | return std::move(t);\n> > > > | ~~~~~~~~~^~~\n> > > > move2.cpp:27:25: note: remove ‘std::move’ call\n> > > > Base::Base()\n> > > > Base::Base(Base &&)\n> > > > Base::Base()\n> > > > Base::Base(Base &&)\n> > > > \n> > > > This is annoying. The move seems to be redundant indeed with g++-13, but\n> > > > dropping it results in the copy constructor being used with g++-12 and\n> > > > earlier.\n> > > \n> > > Worse, I tried\n> > > \n> > > #pragma GCC diagnostic push\n> > > #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> > > /*\n> > > * gcc-13 calls the py::object move constructor even without a\n> > > * std::move(), making it redundant, but earlier gcc versions\n> > > * call the copy constructor. Keep the move and silence the\n> > > * warning.\n> > > */\n> > > return std::move(t);\n> > > #pragma GCC diagnostic pop\n> > > \n> > > which gave me\n> > > \n> > > ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’:\n> > > ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]\n> > > #pragma GCC diagnostic ignored \"-Wredundant-move\"\n> > > ^~~~~~~~~~~~~~~~~~\n> > \n> > Given the pains here, can we just add -Wno-redundant-move or such for\n> > 8.5+ ?\n> \n> I think it got introduced in 9.1, and I think we can disable it, yes. As\n> far I understand the redundant moves don't cause any adverse effect.\n\nI've sent a patch, see https://patchwork.libcamera.org/patch/18575/\n\n> > > with gcc-8.5.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 0D78CBDCBD\n\tfor ;\n\tMon, 1 May 2023 03:16:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72989627E0;\n\tMon, 1 May 2023 05:16:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43FB361EAB\n\tfor ;\n\tMon, 1 May 2023 05:16:49 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7099F27C;\n\tMon, 1 May 2023 05:16:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682911011;\n\tbh=VibAija1X86FK7ChVbFN+mA1HBuKq7Qjlwbk4URCb7M=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=RXjY026gkmGdC5D2yTAcqxHmGutias83Ua+3D7vtOXfekLmNW9DG+JjPEI1vPsChD\n\tn4vAg7HFNW55CQREy38sl+L60qIV8a0pn8v6k6OoDc0Y8FvD5mR8tnPTAjcWNUtLml\n\te14XwEgLK4KFNBGJSECJhbDB5I/Cw0lMys2jnxOwrdBnYWfCmNqQCp7KX4CvPKNtOi\n\tgqXHn8/9fM1ivxnyB7gz+5tkfsK828x3CR3OAHy3noljQrAnwtk6vTGk47LhPpJsxC\n\tIjjzFzPCosbnNYUKhWGbRDlnn528+TgA1XwZWlaDPmvF0dwsEG88lYv4lUcH3gWTER\n\tXCclmeh3htqBQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682911008;\n\tbh=VibAija1X86FK7ChVbFN+mA1HBuKq7Qjlwbk4URCb7M=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=RR6NhWVwGU7H+U/Anec4dZZJXwD9ePylWYqmyvJ8C9zAsiQsUxsjXuF44G4yjinBZ\n\tPY06coveRSrj83Vr21ObUXwdFszaKYmaFd4CwSXCb36636VuRadBj87hMjhipw72Uk\n\tiLJAX6D93E6bpnAQeZG89k5xjfb4se5XM9iE+RFc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RR6NhWVw\"; dkim-atps=neutral","Date":"Mon, 1 May 2023 06:16:59 +0300","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20230501031659.GB19700@pendragon.ideasonboard.com>","References":"<20230124233624.2058-1-laurent.pinchart@ideasonboard.com>\n\t<167460538048.1334668.71671031608644644@Monstersaurus>\n\t\n\t<4c6fcb3b-4690-eab9-bd3e-09232a5815df@ideasonboard.com>\n\t<20230428145107.GD13163@pendragon.ideasonboard.com>\n\t<20230428150808.GA2625@pendragon.ideasonboard.com>\n\t<168274614085.3274474.10017313386842341084@Monstersaurus>\n\t<20230429160321.GA19700@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20230429160321.GA19700@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] py: Drop redundant std::move()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]