| Message ID | 20230124233624.2058-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > gcc-13 warns that the valueOrTuple() function has a redundant > std::move() in a return statement: > > ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > 28 | return std::move(t); ohhh - this may be just too pedantic for me. Explicitly stating std::move(t) when the compiler knows it is a move may be redundant to the compiler, but it's not redundant to the reader?! Doesn't this help make it clear that the t is being moved... in which case it's helpful self documenting code? I'm normally all for warnings, but this one is annoying. https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c states that this isn't a 'pessimizing' operation, it's just redundant, but it does make it clearer that a move is expected to occur? > > Drop it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/py/libcamera/py_helpers.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > index 79891ab63862..5bedea047e31 100644 > --- a/src/py/libcamera/py_helpers.cpp > +++ b/src/py/libcamera/py_helpers.cpp > @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv) > for (size_t i = 0; i < cv.numElements(); ++i) > t[i] = v[i]; > > - return std::move(t); > + return t; > } > > return py::cast(cv.get<T>()); > > base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63 > prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0 > prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42 > prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391 > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > > gcc-13 warns that the valueOrTuple() function has a redundant > > std::move() in a return statement: > > > > ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > > ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > > ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > > 28 | return std::move(t); > > ohhh - this may be just too pedantic for me. Explicitly stating > std::move(t) when the compiler knows it is a move may be redundant to > the compiler, but it's not redundant to the reader?! > > Doesn't this help make it clear that the t is being moved... in which > case it's helpful self documenting code? > > I'm normally all for warnings, but this one is annoying. > > https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > states that this isn't a 'pessimizing' operation, it's just redundant, > but it does make it clearer that a move is expected to occur? Adding more context, the function is implemented as if (cv.isArray()) { const T *v = reinterpret_cast<const T *>(cv.data().data()); auto t = py::tuple(cv.numElements()); for (size_t i = 0; i < cv.numElements(); ++i) t[i] = v[i]; return std::move(t); } return py::cast(cv.get<T>()); The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still produces the same warning), which inherits from py::object. We thus seem to be in the last case described by the above link: There are situations where returning std::move (expr) makes sense, however. The rules for the implicit move require that the selected constructor take an rvalue reference to the returned object's type. Sometimes that isn't the case. For example, when a function returns an object whose type is a class derived from the class type the function returns. In that case, overload resolution is performed a second time, this time treating the object as an lvalue: struct U { }; struct T : U { }; U f() { T t; return std::move (t); } g++-13 produces a warning when compiling that code: move.cpp: In function ‘U f()’: move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] 9 | return std::move (t); | ~~~~~~~~~~^~~ move.cpp:9:26: note: remove ‘std::move’ call This may also be a false positive of gcc ? > > Drop it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/py/libcamera/py_helpers.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > > index 79891ab63862..5bedea047e31 100644 > > --- a/src/py/libcamera/py_helpers.cpp > > +++ b/src/py/libcamera/py_helpers.cpp > > @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv) > > for (size_t i = 0; i < cv.numElements(); ++i) > > t[i] = v[i]; > > > > - return std::move(t); > > + return t; > > } > > > > return py::cast(cv.get<T>()); > > > > base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63 > > prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0 > > prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42 > > prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391
On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > Hi Kieran, > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) >>> gcc-13 warns that the valueOrTuple() function has a redundant >>> std::move() in a return statement: >>> >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] >>> 28 | return std::move(t); >> >> ohhh - this may be just too pedantic for me. Explicitly stating >> std::move(t) when the compiler knows it is a move may be redundant to >> the compiler, but it's not redundant to the reader?! >> >> Doesn't this help make it clear that the t is being moved... in which >> case it's helpful self documenting code? >> >> I'm normally all for warnings, but this one is annoying. >> >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c >> states that this isn't a 'pessimizing' operation, it's just redundant, >> but it does make it clearer that a move is expected to occur? > > Adding more context, the function is implemented as > > if (cv.isArray()) { > const T *v = reinterpret_cast<const T *>(cv.data().data()); > auto t = py::tuple(cv.numElements()); > > for (size_t i = 0; i < cv.numElements(); ++i) > t[i] = v[i]; > > return std::move(t); > } > > return py::cast(cv.get<T>()); > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > produces the same warning), which inherits from py::object. We thus seem > to be in the last case described by the above link: > > There are situations where returning std::move (expr) makes sense, > however. The rules for the implicit move require that the selected > constructor take an rvalue reference to the returned object's type. > Sometimes that isn't the case. For example, when a function returns > an object whose type is a class derived from the class type the > function returns. In that case, overload resolution is performed a > second time, this time treating the object as an lvalue: > > struct U { }; > struct T : U { }; > > U f() > { > T t; > return std::move (t); > } > > g++-13 produces a warning when compiling that code: > > move.cpp: In function ‘U f()’: > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > 9 | return std::move (t); > | ~~~~~~~~~~^~~ > move.cpp:9:26: note: remove ‘std::move’ call > > This may also be a false positive of gcc ? I don't have gcc 13, nor does godbolt.org, but other gcc nor clang versions don't complain. With some testing on godbolt, with and without std::move the end result is the same (with -O2) on the compilers I tested. So... I don't know. The text you pasted seems to suggest that std::move() would be needed there, but I don't see a diff (then again, my test code is just test code, not the actual py code we have). I'm fine either way, but if gcc 13 is not much used yet, maybe we should wait a bit? Also, a bit beside the point, I'm actually a bit surprised that U f() { return T(); } works without warnings (even if I add fields to U and T). It's silently throwing away the T specific parts, only keeping the U parts. Tomi
On 05/04/2023 09:49, Tomi Valkeinen via libcamera-devel wrote: > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: >> Hi Kieran, >> >> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: >>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) >>>> gcc-13 warns that the valueOrTuple() function has a redundant >>>> std::move() in a return statement: >>>> >>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of >>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with >>>> T = bool]’: >>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here >>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move >>>> in return statement [-Werror=redundant-move] >>>> 28 | return std::move(t); >>> >>> ohhh - this may be just too pedantic for me. Explicitly stating >>> std::move(t) when the compiler knows it is a move may be redundant to >>> the compiler, but it's not redundant to the reader?! >>> >>> Doesn't this help make it clear that the t is being moved... in which >>> case it's helpful self documenting code? >>> >>> I'm normally all for warnings, but this one is annoying. >>> >>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c >>> states that this isn't a 'pessimizing' operation, it's just redundant, >>> but it does make it clearer that a move is expected to occur? >> >> Adding more context, the function is implemented as >> >> if (cv.isArray()) { >> const T *v = reinterpret_cast<const T *>(cv.data().data()); >> auto t = py::tuple(cv.numElements()); >> >> for (size_t i = 0; i < cv.numElements(); ++i) >> t[i] = v[i]; >> >> return std::move(t); >> } >> >> return py::cast(cv.get<T>()); >> >> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still >> produces the same warning), which inherits from py::object. We thus seem >> to be in the last case described by the above link: >> >> There are situations where returning std::move (expr) makes sense, >> however. The rules for the implicit move require that the selected >> constructor take an rvalue reference to the returned object's type. >> Sometimes that isn't the case. For example, when a function returns >> an object whose type is a class derived from the class type the >> function returns. In that case, overload resolution is performed a >> second time, this time treating the object as an lvalue: >> >> struct U { }; >> struct T : U { }; >> >> U f() >> { >> T t; >> return std::move (t); >> } >> >> g++-13 produces a warning when compiling that code: >> >> move.cpp: In function ‘U f()’: >> move.cpp:9:26: warning: redundant move in return statement >> [-Wredundant-move] >> 9 | return std::move (t); >> | ~~~~~~~~~~^~~ >> move.cpp:9:26: note: remove ‘std::move’ call >> >> This may also be a false positive of gcc ? > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > versions don't complain. > > With some testing on godbolt, with and without std::move the end result > is the same (with -O2) on the compilers I tested. > > So... I don't know. The text you pasted seems to suggest that > std::move() would be needed there, but I don't see a diff (then again, > my test code is just test code, not the actual py code we have). I'm > fine either way, but if gcc 13 is not much used yet, maybe we should > wait a bit? > > Also, a bit beside the point, I'm actually a bit surprised that > > U f() > { > return T(); > } > > works without warnings (even if I add fields to U and T). It's silently > throwing away the T specific parts, only keeping the U parts. > > Tomi > Maybe add a comment, noting that this used to have std::move(), but it was dropped as gcc 13 warns. And we're not sure if the warning is correct. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
Hi Tomi, On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > >>> gcc-13 warns that the valueOrTuple() function has a redundant > >>> std::move() in a return statement: > >>> > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > >>> 28 | return std::move(t); > >> > >> ohhh - this may be just too pedantic for me. Explicitly stating > >> std::move(t) when the compiler knows it is a move may be redundant to > >> the compiler, but it's not redundant to the reader?! > >> > >> Doesn't this help make it clear that the t is being moved... in which > >> case it's helpful self documenting code? > >> > >> I'm normally all for warnings, but this one is annoying. > >> > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > >> states that this isn't a 'pessimizing' operation, it's just redundant, > >> but it does make it clearer that a move is expected to occur? > > > > Adding more context, the function is implemented as > > > > if (cv.isArray()) { > > const T *v = reinterpret_cast<const T *>(cv.data().data()); > > auto t = py::tuple(cv.numElements()); > > > > for (size_t i = 0; i < cv.numElements(); ++i) > > t[i] = v[i]; > > > > return std::move(t); > > } > > > > return py::cast(cv.get<T>()); > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > > produces the same warning), which inherits from py::object. We thus seem > > to be in the last case described by the above link: > > > > There are situations where returning std::move (expr) makes sense, > > however. The rules for the implicit move require that the selected > > constructor take an rvalue reference to the returned object's type. > > Sometimes that isn't the case. For example, when a function returns > > an object whose type is a class derived from the class type the > > function returns. In that case, overload resolution is performed a > > second time, this time treating the object as an lvalue: > > > > struct U { }; > > struct T : U { }; > > > > U f() > > { > > T t; > > return std::move (t); > > } > > > > g++-13 produces a warning when compiling that code: > > > > move.cpp: In function ‘U f()’: > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > > 9 | return std::move (t); > > | ~~~~~~~~~~^~~ > > move.cpp:9:26: note: remove ‘std::move’ call > > > > This may also be a false positive of gcc ? > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > versions don't complain. > > With some testing on godbolt, with and without std::move the end result > is the same (with -O2) on the compilers I tested. > > So... I don't know. The text you pasted seems to suggest that > std::move() would be needed there, but I don't see a diff (then again, > my test code is just test code, not the actual py code we have). I'm > fine either way, but if gcc 13 is not much used yet, maybe we should > wait a bit? > > Also, a bit beside the point, I'm actually a bit surprised that > > U f() > { > return T(); > } > > works without warnings (even if I add fields to U and T). It's silently > throwing away the T specific parts, only keeping the U parts. I tried this test code: -------- #include <functional> #include <iostream> struct Base { Base() { std::cout << "Base::Base()" << std::endl; } Base(const Base &) { std::cout << "Base::Base(const Base &)" << std::endl; } Base(Base &&) { std::cout << "Base::Base(Base &&)" << std::endl; } }; struct Derived : Base { }; Base f() { Derived t; return std::move(t); } Base g() { Derived t; return t; } int main() { f(); g(); return 0; } -------- $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 Base::Base() Base::Base(Base &&) Base::Base() Base::Base(const Base &) $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 move2.cpp: In function ‘Base f()’: move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] 27 | return std::move(t); | ~~~~~~~~~^~~ move2.cpp:27:25: note: remove ‘std::move’ call Base::Base() Base::Base(Base &&) Base::Base() Base::Base(Base &&) This is annoying. The move seems to be redundant indeed with g++-13, but dropping it results in the copy constructor being used with g++-12 and earlier.
On 28/04/2023 17:51, Laurent Pinchart wrote: > Hi Tomi, > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: >> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: >>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: >>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) >>>>> gcc-13 warns that the valueOrTuple() function has a redundant >>>>> std::move() in a return statement: >>>>> >>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: >>>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here >>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] >>>>> 28 | return std::move(t); >>>> >>>> ohhh - this may be just too pedantic for me. Explicitly stating >>>> std::move(t) when the compiler knows it is a move may be redundant to >>>> the compiler, but it's not redundant to the reader?! >>>> >>>> Doesn't this help make it clear that the t is being moved... in which >>>> case it's helpful self documenting code? >>>> >>>> I'm normally all for warnings, but this one is annoying. >>>> >>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c >>>> states that this isn't a 'pessimizing' operation, it's just redundant, >>>> but it does make it clearer that a move is expected to occur? >>> >>> Adding more context, the function is implemented as >>> >>> if (cv.isArray()) { >>> const T *v = reinterpret_cast<const T *>(cv.data().data()); >>> auto t = py::tuple(cv.numElements()); >>> >>> for (size_t i = 0; i < cv.numElements(); ++i) >>> t[i] = v[i]; >>> >>> return std::move(t); >>> } >>> >>> return py::cast(cv.get<T>()); >>> >>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still >>> produces the same warning), which inherits from py::object. We thus seem >>> to be in the last case described by the above link: >>> >>> There are situations where returning std::move (expr) makes sense, >>> however. The rules for the implicit move require that the selected >>> constructor take an rvalue reference to the returned object's type. >>> Sometimes that isn't the case. For example, when a function returns >>> an object whose type is a class derived from the class type the >>> function returns. In that case, overload resolution is performed a >>> second time, this time treating the object as an lvalue: >>> >>> struct U { }; >>> struct T : U { }; >>> >>> U f() >>> { >>> T t; >>> return std::move (t); >>> } >>> >>> g++-13 produces a warning when compiling that code: >>> >>> move.cpp: In function ‘U f()’: >>> move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] >>> 9 | return std::move (t); >>> | ~~~~~~~~~~^~~ >>> move.cpp:9:26: note: remove ‘std::move’ call >>> >>> This may also be a false positive of gcc ? >> >> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang >> versions don't complain. >> >> With some testing on godbolt, with and without std::move the end result >> is the same (with -O2) on the compilers I tested. >> >> So... I don't know. The text you pasted seems to suggest that >> std::move() would be needed there, but I don't see a diff (then again, >> my test code is just test code, not the actual py code we have). I'm >> fine either way, but if gcc 13 is not much used yet, maybe we should >> wait a bit? >> >> Also, a bit beside the point, I'm actually a bit surprised that >> >> U f() >> { >> return T(); >> } >> >> works without warnings (even if I add fields to U and T). It's silently >> throwing away the T specific parts, only keeping the U parts. > > I tried this test code: > > -------- > #include <functional> > #include <iostream> > > struct Base { > Base() > { > std::cout << "Base::Base()" << std::endl; > } > > Base(const Base &) > { > std::cout << "Base::Base(const Base &)" << std::endl; > } > > Base(Base &&) > { > std::cout << "Base::Base(Base &&)" << std::endl; > } > }; > > struct Derived : Base { > }; > > Base f() > { > Derived t; > return std::move(t); > } > > Base g() > { > Derived t; > return t; > } > > int main() > { > f(); > g(); > > return 0; > } > -------- > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 > Base::Base() > Base::Base(Base &&) > Base::Base() > Base::Base(const Base &) > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 > move2.cpp: In function ‘Base f()’: > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] > 27 | return std::move(t); > | ~~~~~~~~~^~~ > move2.cpp:27:25: note: remove ‘std::move’ call > Base::Base() > Base::Base(Base &&) > Base::Base() > Base::Base(Base &&) > > This is annoying. The move seems to be redundant indeed with g++-13, but > dropping it results in the copy constructor being used with g++-12 and > earlier. > What about: Derived t; Base& b = t; return std::move(b); Tomi
On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote: > Hi Tomi, > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > > >>> gcc-13 warns that the valueOrTuple() function has a redundant > > >>> std::move() in a return statement: > > >>> > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > > >>> 28 | return std::move(t); > > >> > > >> ohhh - this may be just too pedantic for me. Explicitly stating > > >> std::move(t) when the compiler knows it is a move may be redundant to > > >> the compiler, but it's not redundant to the reader?! > > >> > > >> Doesn't this help make it clear that the t is being moved... in which > > >> case it's helpful self documenting code? > > >> > > >> I'm normally all for warnings, but this one is annoying. > > >> > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > > >> states that this isn't a 'pessimizing' operation, it's just redundant, > > >> but it does make it clearer that a move is expected to occur? > > > > > > Adding more context, the function is implemented as > > > > > > if (cv.isArray()) { > > > const T *v = reinterpret_cast<const T *>(cv.data().data()); > > > auto t = py::tuple(cv.numElements()); > > > > > > for (size_t i = 0; i < cv.numElements(); ++i) > > > t[i] = v[i]; > > > > > > return std::move(t); > > > } > > > > > > return py::cast(cv.get<T>()); > > > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > > > produces the same warning), which inherits from py::object. We thus seem > > > to be in the last case described by the above link: > > > > > > There are situations where returning std::move (expr) makes sense, > > > however. The rules for the implicit move require that the selected > > > constructor take an rvalue reference to the returned object's type. > > > Sometimes that isn't the case. For example, when a function returns > > > an object whose type is a class derived from the class type the > > > function returns. In that case, overload resolution is performed a > > > second time, this time treating the object as an lvalue: > > > > > > struct U { }; > > > struct T : U { }; > > > > > > U f() > > > { > > > T t; > > > return std::move (t); > > > } > > > > > > g++-13 produces a warning when compiling that code: > > > > > > move.cpp: In function ‘U f()’: > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > > > 9 | return std::move (t); > > > | ~~~~~~~~~~^~~ > > > move.cpp:9:26: note: remove ‘std::move’ call > > > > > > This may also be a false positive of gcc ? > > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > > versions don't complain. > > > > With some testing on godbolt, with and without std::move the end result > > is the same (with -O2) on the compilers I tested. > > > > So... I don't know. The text you pasted seems to suggest that > > std::move() would be needed there, but I don't see a diff (then again, > > my test code is just test code, not the actual py code we have). I'm > > fine either way, but if gcc 13 is not much used yet, maybe we should > > wait a bit? > > > > Also, a bit beside the point, I'm actually a bit surprised that > > > > U f() > > { > > return T(); > > } > > > > works without warnings (even if I add fields to U and T). It's silently > > throwing away the T specific parts, only keeping the U parts. > > I tried this test code: > > -------- > #include <functional> > #include <iostream> > > struct Base { > Base() > { > std::cout << "Base::Base()" << std::endl; > } > > Base(const Base &) > { > std::cout << "Base::Base(const Base &)" << std::endl; > } > > Base(Base &&) > { > std::cout << "Base::Base(Base &&)" << std::endl; > } > }; > > struct Derived : Base { > }; > > Base f() > { > Derived t; > return std::move(t); > } > > Base g() > { > Derived t; > return t; > } > > int main() > { > f(); > g(); > > return 0; > } > -------- > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 > Base::Base() > Base::Base(Base &&) > Base::Base() > Base::Base(const Base &) > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 > move2.cpp: In function ‘Base f()’: > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] > 27 | return std::move(t); > | ~~~~~~~~~^~~ > move2.cpp:27:25: note: remove ‘std::move’ call > Base::Base() > Base::Base(Base &&) > Base::Base() > Base::Base(Base &&) > > This is annoying. The move seems to be redundant indeed with g++-13, but > dropping it results in the copy constructor being used with g++-12 and > earlier. Worse, I tried #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wredundant-move" /* * gcc-13 calls the py::object move constructor even without a * std::move(), making it redundant, but earlier gcc versions * call the copy constructor. Keep the move and silence the * warning. */ return std::move(t); #pragma GCC diagnostic pop which gave me ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’: ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas] #pragma GCC diagnostic ignored "-Wredundant-move" ^~~~~~~~~~~~~~~~~~ with gcc-8.5.0.
On 28/04/2023 18:03, Tomi Valkeinen wrote: > On 28/04/2023 17:51, Laurent Pinchart wrote: >> Hi Tomi, >> >> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: >>> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: >>>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: >>>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) >>>>>> gcc-13 warns that the valueOrTuple() function has a redundant >>>>>> std::move() in a return statement: >>>>>> >>>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of >>>>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) >>>>>> [with T = bool]’: >>>>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here >>>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move >>>>>> in return statement [-Werror=redundant-move] >>>>>> 28 | return std::move(t); >>>>> >>>>> ohhh - this may be just too pedantic for me. Explicitly stating >>>>> std::move(t) when the compiler knows it is a move may be redundant to >>>>> the compiler, but it's not redundant to the reader?! >>>>> >>>>> Doesn't this help make it clear that the t is being moved... in which >>>>> case it's helpful self documenting code? >>>>> >>>>> I'm normally all for warnings, but this one is annoying. >>>>> >>>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c >>>>> states that this isn't a 'pessimizing' operation, it's just redundant, >>>>> but it does make it clearer that a move is expected to occur? >>>> >>>> Adding more context, the function is implemented as >>>> >>>> if (cv.isArray()) { >>>> const T *v = reinterpret_cast<const T *>(cv.data().data()); >>>> auto t = py::tuple(cv.numElements()); >>>> >>>> for (size_t i = 0; i < cv.numElements(); ++i) >>>> t[i] = v[i]; >>>> >>>> return std::move(t); >>>> } >>>> >>>> return py::cast(cv.get<T>()); >>>> >>>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still >>>> produces the same warning), which inherits from py::object. We thus >>>> seem >>>> to be in the last case described by the above link: >>>> >>>> There are situations where returning std::move (expr) makes >>>> sense, >>>> however. The rules for the implicit move require that the >>>> selected >>>> constructor take an rvalue reference to the returned object's >>>> type. >>>> Sometimes that isn't the case. For example, when a function >>>> returns >>>> an object whose type is a class derived from the class type the >>>> function returns. In that case, overload resolution is >>>> performed a >>>> second time, this time treating the object as an lvalue: >>>> >>>> struct U { }; >>>> struct T : U { }; >>>> >>>> U f() >>>> { >>>> T t; >>>> return std::move (t); >>>> } >>>> >>>> g++-13 produces a warning when compiling that code: >>>> >>>> move.cpp: In function ‘U f()’: >>>> move.cpp:9:26: warning: redundant move in return statement >>>> [-Wredundant-move] >>>> 9 | return std::move (t); >>>> | ~~~~~~~~~~^~~ >>>> move.cpp:9:26: note: remove ‘std::move’ call >>>> >>>> This may also be a false positive of gcc ? >>> >>> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang >>> versions don't complain. >>> >>> With some testing on godbolt, with and without std::move the end result >>> is the same (with -O2) on the compilers I tested. >>> >>> So... I don't know. The text you pasted seems to suggest that >>> std::move() would be needed there, but I don't see a diff (then again, >>> my test code is just test code, not the actual py code we have). I'm >>> fine either way, but if gcc 13 is not much used yet, maybe we should >>> wait a bit? >>> >>> Also, a bit beside the point, I'm actually a bit surprised that >>> >>> U f() >>> { >>> return T(); >>> } >>> >>> works without warnings (even if I add fields to U and T). It's silently >>> throwing away the T specific parts, only keeping the U parts. >> >> I tried this test code: >> >> -------- >> #include <functional> >> #include <iostream> >> >> struct Base { >> Base() >> { >> std::cout << "Base::Base()" << std::endl; >> } >> >> Base(const Base &) >> { >> std::cout << "Base::Base(const Base &)" << std::endl; >> } >> >> Base(Base &&) >> { >> std::cout << "Base::Base(Base &&)" << std::endl; >> } >> }; >> >> struct Derived : Base { >> }; >> >> Base f() >> { >> Derived t; >> return std::move(t); >> } >> >> Base g() >> { >> Derived t; >> return t; >> } >> >> int main() >> { >> f(); >> g(); >> >> return 0; >> } >> -------- >> >> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 >> Base::Base() >> Base::Base(Base &&) >> Base::Base() >> Base::Base(const Base &) >> >> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 >> move2.cpp: In function ‘Base f()’: >> move2.cpp:27:25: warning: redundant move in return statement >> [-Wredundant-move] >> 27 | return std::move(t); >> | ~~~~~~~~~^~~ >> move2.cpp:27:25: note: remove ‘std::move’ call >> Base::Base() >> Base::Base(Base &&) >> Base::Base() >> Base::Base(Base &&) >> >> This is annoying. The move seems to be redundant indeed with g++-13, but >> dropping it results in the copy constructor being used with g++-12 and >> earlier. >> > > What about: > > Derived t; > Base& b = t; > return std::move(b); > Ah, nice, godbolt now has gcc 13. The above seems to compile fine on gcc 13, and doesn't seem to change the produced code on gcc 12 and gcc 13. Tomi
On 28/04/2023 18:10, Tomi Valkeinen wrote: > On 28/04/2023 18:03, Tomi Valkeinen wrote: >> On 28/04/2023 17:51, Laurent Pinchart wrote: >>> Hi Tomi, >>> >>> On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: >>>> On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: >>>>> On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: >>>>>> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) >>>>>>> gcc-13 warns that the valueOrTuple() function has a redundant >>>>>>> std::move() in a return statement: >>>>>>> >>>>>>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of >>>>>>> ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) >>>>>>> [with T = bool]’: >>>>>>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here >>>>>>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant >>>>>>> move in return statement [-Werror=redundant-move] >>>>>>> 28 | return std::move(t); >>>>>> >>>>>> ohhh - this may be just too pedantic for me. Explicitly stating >>>>>> std::move(t) when the compiler knows it is a move may be redundant to >>>>>> the compiler, but it's not redundant to the reader?! >>>>>> >>>>>> Doesn't this help make it clear that the t is being moved... in which >>>>>> case it's helpful self documenting code? >>>>>> >>>>>> I'm normally all for warnings, but this one is annoying. >>>>>> >>>>>> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c >>>>>> states that this isn't a 'pessimizing' operation, it's just >>>>>> redundant, >>>>>> but it does make it clearer that a move is expected to occur? >>>>> >>>>> Adding more context, the function is implemented as >>>>> >>>>> if (cv.isArray()) { >>>>> const T *v = reinterpret_cast<const T *>(cv.data().data()); >>>>> auto t = py::tuple(cv.numElements()); >>>>> >>>>> for (size_t i = 0; i < cv.numElements(); ++i) >>>>> t[i] = v[i]; >>>>> >>>>> return std::move(t); >>>>> } >>>>> >>>>> return py::cast(cv.get<T>()); >>>>> >>>>> The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still >>>>> produces the same warning), which inherits from py::object. We thus >>>>> seem >>>>> to be in the last case described by the above link: >>>>> >>>>> There are situations where returning std::move (expr) makes >>>>> sense, >>>>> however. The rules for the implicit move require that the >>>>> selected >>>>> constructor take an rvalue reference to the returned object's >>>>> type. >>>>> Sometimes that isn't the case. For example, when a function >>>>> returns >>>>> an object whose type is a class derived from the class type the >>>>> function returns. In that case, overload resolution is >>>>> performed a >>>>> second time, this time treating the object as an lvalue: >>>>> >>>>> struct U { }; >>>>> struct T : U { }; >>>>> >>>>> U f() >>>>> { >>>>> T t; >>>>> return std::move (t); >>>>> } >>>>> >>>>> g++-13 produces a warning when compiling that code: >>>>> >>>>> move.cpp: In function ‘U f()’: >>>>> move.cpp:9:26: warning: redundant move in return statement >>>>> [-Wredundant-move] >>>>> 9 | return std::move (t); >>>>> | ~~~~~~~~~~^~~ >>>>> move.cpp:9:26: note: remove ‘std::move’ call >>>>> >>>>> This may also be a false positive of gcc ? >>>> >>>> I don't have gcc 13, nor does godbolt.org, but other gcc nor clang >>>> versions don't complain. >>>> >>>> With some testing on godbolt, with and without std::move the end result >>>> is the same (with -O2) on the compilers I tested. >>>> >>>> So... I don't know. The text you pasted seems to suggest that >>>> std::move() would be needed there, but I don't see a diff (then again, >>>> my test code is just test code, not the actual py code we have). I'm >>>> fine either way, but if gcc 13 is not much used yet, maybe we should >>>> wait a bit? >>>> >>>> Also, a bit beside the point, I'm actually a bit surprised that >>>> >>>> U f() >>>> { >>>> return T(); >>>> } >>>> >>>> works without warnings (even if I add fields to U and T). It's silently >>>> throwing away the T specific parts, only keeping the U parts. >>> >>> I tried this test code: >>> >>> -------- >>> #include <functional> >>> #include <iostream> >>> >>> struct Base { >>> Base() >>> { >>> std::cout << "Base::Base()" << std::endl; >>> } >>> >>> Base(const Base &) >>> { >>> std::cout << "Base::Base(const Base &)" << std::endl; >>> } >>> >>> Base(Base &&) >>> { >>> std::cout << "Base::Base(Base &&)" << std::endl; >>> } >>> }; >>> >>> struct Derived : Base { >>> }; >>> >>> Base f() >>> { >>> Derived t; >>> return std::move(t); >>> } >>> >>> Base g() >>> { >>> Derived t; >>> return t; >>> } >>> >>> int main() >>> { >>> f(); >>> g(); >>> >>> return 0; >>> } >>> -------- >>> >>> $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 >>> Base::Base() >>> Base::Base(Base &&) >>> Base::Base() >>> Base::Base(const Base &) >>> >>> $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 >>> move2.cpp: In function ‘Base f()’: >>> move2.cpp:27:25: warning: redundant move in return statement >>> [-Wredundant-move] >>> 27 | return std::move(t); >>> | ~~~~~~~~~^~~ >>> move2.cpp:27:25: note: remove ‘std::move’ call >>> Base::Base() >>> Base::Base(Base &&) >>> Base::Base() >>> Base::Base(Base &&) >>> >>> This is annoying. The move seems to be redundant indeed with g++-13, but >>> dropping it results in the copy constructor being used with g++-12 and >>> earlier. >>> >> >> What about: >> >> Derived t; >> Base& b = t; >> return std::move(b); >> > > Ah, nice, godbolt now has gcc 13. > > The above seems to compile fine on gcc 13, and doesn't seem to change > the produced code on gcc 12 and gcc 13. > > Tomi > Or this also seems to do the trick: return static_cast<Base&&>(std::move(t)); Tomi
Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08) > On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote: > > Hi Tomi, > > > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: > > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > > > >>> gcc-13 warns that the valueOrTuple() function has a redundant > > > >>> std::move() in a return statement: > > > >>> > > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > > > >>> 28 | return std::move(t); > > > >> > > > >> ohhh - this may be just too pedantic for me. Explicitly stating > > > >> std::move(t) when the compiler knows it is a move may be redundant to > > > >> the compiler, but it's not redundant to the reader?! > > > >> > > > >> Doesn't this help make it clear that the t is being moved... in which > > > >> case it's helpful self documenting code? > > > >> > > > >> I'm normally all for warnings, but this one is annoying. > > > >> > > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > > > >> states that this isn't a 'pessimizing' operation, it's just redundant, > > > >> but it does make it clearer that a move is expected to occur? > > > > > > > > Adding more context, the function is implemented as > > > > > > > > if (cv.isArray()) { > > > > const T *v = reinterpret_cast<const T *>(cv.data().data()); > > > > auto t = py::tuple(cv.numElements()); > > > > > > > > for (size_t i = 0; i < cv.numElements(); ++i) > > > > t[i] = v[i]; > > > > > > > > return std::move(t); > > > > } > > > > > > > > return py::cast(cv.get<T>()); > > > > > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > > > > produces the same warning), which inherits from py::object. We thus seem > > > > to be in the last case described by the above link: > > > > > > > > There are situations where returning std::move (expr) makes sense, > > > > however. The rules for the implicit move require that the selected > > > > constructor take an rvalue reference to the returned object's type. > > > > Sometimes that isn't the case. For example, when a function returns > > > > an object whose type is a class derived from the class type the > > > > function returns. In that case, overload resolution is performed a > > > > second time, this time treating the object as an lvalue: > > > > > > > > struct U { }; > > > > struct T : U { }; > > > > > > > > U f() > > > > { > > > > T t; > > > > return std::move (t); > > > > } > > > > > > > > g++-13 produces a warning when compiling that code: > > > > > > > > move.cpp: In function ‘U f()’: > > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > > > > 9 | return std::move (t); > > > > | ~~~~~~~~~~^~~ > > > > move.cpp:9:26: note: remove ‘std::move’ call > > > > > > > > This may also be a false positive of gcc ? > > > > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > > > versions don't complain. > > > > > > With some testing on godbolt, with and without std::move the end result > > > is the same (with -O2) on the compilers I tested. > > > > > > So... I don't know. The text you pasted seems to suggest that > > > std::move() would be needed there, but I don't see a diff (then again, > > > my test code is just test code, not the actual py code we have). I'm > > > fine either way, but if gcc 13 is not much used yet, maybe we should > > > wait a bit? > > > > > > Also, a bit beside the point, I'm actually a bit surprised that > > > > > > U f() > > > { > > > return T(); > > > } > > > > > > works without warnings (even if I add fields to U and T). It's silently > > > throwing away the T specific parts, only keeping the U parts. > > > > I tried this test code: > > > > -------- > > #include <functional> > > #include <iostream> > > > > struct Base { > > Base() > > { > > std::cout << "Base::Base()" << std::endl; > > } > > > > Base(const Base &) > > { > > std::cout << "Base::Base(const Base &)" << std::endl; > > } > > > > Base(Base &&) > > { > > std::cout << "Base::Base(Base &&)" << std::endl; > > } > > }; > > > > struct Derived : Base { > > }; > > > > Base f() > > { > > Derived t; > > return std::move(t); > > } > > > > Base g() > > { > > Derived t; > > return t; > > } > > > > int main() > > { > > f(); > > g(); > > > > return 0; > > } > > -------- > > > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 > > Base::Base() > > Base::Base(Base &&) > > Base::Base() > > Base::Base(const Base &) > > > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 > > move2.cpp: In function ‘Base f()’: > > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] > > 27 | return std::move(t); > > | ~~~~~~~~~^~~ > > move2.cpp:27:25: note: remove ‘std::move’ call > > Base::Base() > > Base::Base(Base &&) > > Base::Base() > > Base::Base(Base &&) > > > > This is annoying. The move seems to be redundant indeed with g++-13, but > > dropping it results in the copy constructor being used with g++-12 and > > earlier. > > Worse, I tried > > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored "-Wredundant-move" > /* > * gcc-13 calls the py::object move constructor even without a > * std::move(), making it redundant, but earlier gcc versions > * call the copy constructor. Keep the move and silence the > * warning. > */ > return std::move(t); > #pragma GCC diagnostic pop > > which gave me > > ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’: > ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas] > #pragma GCC diagnostic ignored "-Wredundant-move" > ^~~~~~~~~~~~~~~~~~ > Given the pains here, can we just add -Wno-redundant-move or such for 8.5+ ? -- Kieran > with gcc-8.5.0. > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Sat, Apr 29, 2023 at 06:29:00AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08) > > On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: > > > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > > > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > > > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > > > > >>> gcc-13 warns that the valueOrTuple() function has a redundant > > > > >>> std::move() in a return statement: > > > > >>> > > > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > > > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > > > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > > > > >>> 28 | return std::move(t); > > > > >> > > > > >> ohhh - this may be just too pedantic for me. Explicitly stating > > > > >> std::move(t) when the compiler knows it is a move may be redundant to > > > > >> the compiler, but it's not redundant to the reader?! > > > > >> > > > > >> Doesn't this help make it clear that the t is being moved... in which > > > > >> case it's helpful self documenting code? > > > > >> > > > > >> I'm normally all for warnings, but this one is annoying. > > > > >> > > > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > > > > >> states that this isn't a 'pessimizing' operation, it's just redundant, > > > > >> but it does make it clearer that a move is expected to occur? > > > > > > > > > > Adding more context, the function is implemented as > > > > > > > > > > if (cv.isArray()) { > > > > > const T *v = reinterpret_cast<const T *>(cv.data().data()); > > > > > auto t = py::tuple(cv.numElements()); > > > > > > > > > > for (size_t i = 0; i < cv.numElements(); ++i) > > > > > t[i] = v[i]; > > > > > > > > > > return std::move(t); > > > > > } > > > > > > > > > > return py::cast(cv.get<T>()); > > > > > > > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > > > > > produces the same warning), which inherits from py::object. We thus seem > > > > > to be in the last case described by the above link: > > > > > > > > > > There are situations where returning std::move (expr) makes sense, > > > > > however. The rules for the implicit move require that the selected > > > > > constructor take an rvalue reference to the returned object's type. > > > > > Sometimes that isn't the case. For example, when a function returns > > > > > an object whose type is a class derived from the class type the > > > > > function returns. In that case, overload resolution is performed a > > > > > second time, this time treating the object as an lvalue: > > > > > > > > > > struct U { }; > > > > > struct T : U { }; > > > > > > > > > > U f() > > > > > { > > > > > T t; > > > > > return std::move (t); > > > > > } > > > > > > > > > > g++-13 produces a warning when compiling that code: > > > > > > > > > > move.cpp: In function ‘U f()’: > > > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > > > > > 9 | return std::move (t); > > > > > | ~~~~~~~~~~^~~ > > > > > move.cpp:9:26: note: remove ‘std::move’ call > > > > > > > > > > This may also be a false positive of gcc ? > > > > > > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > > > > versions don't complain. > > > > > > > > With some testing on godbolt, with and without std::move the end result > > > > is the same (with -O2) on the compilers I tested. > > > > > > > > So... I don't know. The text you pasted seems to suggest that > > > > std::move() would be needed there, but I don't see a diff (then again, > > > > my test code is just test code, not the actual py code we have). I'm > > > > fine either way, but if gcc 13 is not much used yet, maybe we should > > > > wait a bit? > > > > > > > > Also, a bit beside the point, I'm actually a bit surprised that > > > > > > > > U f() > > > > { > > > > return T(); > > > > } > > > > > > > > works without warnings (even if I add fields to U and T). It's silently > > > > throwing away the T specific parts, only keeping the U parts. > > > > > > I tried this test code: > > > > > > -------- > > > #include <functional> > > > #include <iostream> > > > > > > struct Base { > > > Base() > > > { > > > std::cout << "Base::Base()" << std::endl; > > > } > > > > > > Base(const Base &) > > > { > > > std::cout << "Base::Base(const Base &)" << std::endl; > > > } > > > > > > Base(Base &&) > > > { > > > std::cout << "Base::Base(Base &&)" << std::endl; > > > } > > > }; > > > > > > struct Derived : Base { > > > }; > > > > > > Base f() > > > { > > > Derived t; > > > return std::move(t); > > > } > > > > > > Base g() > > > { > > > Derived t; > > > return t; > > > } > > > > > > int main() > > > { > > > f(); > > > g(); > > > > > > return 0; > > > } > > > -------- > > > > > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 > > > Base::Base() > > > Base::Base(Base &&) > > > Base::Base() > > > Base::Base(const Base &) > > > > > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 > > > move2.cpp: In function ‘Base f()’: > > > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] > > > 27 | return std::move(t); > > > | ~~~~~~~~~^~~ > > > move2.cpp:27:25: note: remove ‘std::move’ call > > > Base::Base() > > > Base::Base(Base &&) > > > Base::Base() > > > Base::Base(Base &&) > > > > > > This is annoying. The move seems to be redundant indeed with g++-13, but > > > dropping it results in the copy constructor being used with g++-12 and > > > earlier. > > > > Worse, I tried > > > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored "-Wredundant-move" > > /* > > * gcc-13 calls the py::object move constructor even without a > > * std::move(), making it redundant, but earlier gcc versions > > * call the copy constructor. Keep the move and silence the > > * warning. > > */ > > return std::move(t); > > #pragma GCC diagnostic pop > > > > which gave me > > > > ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’: > > ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas] > > #pragma GCC diagnostic ignored "-Wredundant-move" > > ^~~~~~~~~~~~~~~~~~ > > Given the pains here, can we just add -Wno-redundant-move or such for > 8.5+ ? I think it got introduced in 9.1, and I think we can disable it, yes. As far I understand the redundant moves don't cause any adverse effect. > > with gcc-8.5.0.
On Sat, Apr 29, 2023 at 07:03:21PM +0300, Laurent Pinchart via libcamera-devel wrote: > On Sat, Apr 29, 2023 at 06:29:00AM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart via libcamera-devel (2023-04-28 16:08:08) > > > On Fri, Apr 28, 2023 at 05:51:07PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > On Wed, Apr 05, 2023 at 09:49:45AM +0300, Tomi Valkeinen wrote: > > > > > On 25/01/2023 11:42, Laurent Pinchart via libcamera-devel wrote: > > > > > > On Wed, Jan 25, 2023 at 12:09:40AM +0000, Kieran Bingham wrote: > > > > > >> Quoting Laurent Pinchart via libcamera-devel (2023-01-24 23:36:24) > > > > > >>> gcc-13 warns that the valueOrTuple() function has a redundant > > > > > >>> std::move() in a return statement: > > > > > >>> > > > > > >>> ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: > > > > > >>> ../../src/py/libcamera/py_helpers.cpp:38:28: required from here > > > > > >>> ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] > > > > > >>> 28 | return std::move(t); > > > > > >> > > > > > >> ohhh - this may be just too pedantic for me. Explicitly stating > > > > > >> std::move(t) when the compiler knows it is a move may be redundant to > > > > > >> the compiler, but it's not redundant to the reader?! > > > > > >> > > > > > >> Doesn't this help make it clear that the t is being moved... in which > > > > > >> case it's helpful self documenting code? > > > > > >> > > > > > >> I'm normally all for warnings, but this one is annoying. > > > > > >> > > > > > >> https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c > > > > > >> states that this isn't a 'pessimizing' operation, it's just redundant, > > > > > >> but it does make it clearer that a move is expected to occur? > > > > > > > > > > > > Adding more context, the function is implemented as > > > > > > > > > > > > if (cv.isArray()) { > > > > > > const T *v = reinterpret_cast<const T *>(cv.data().data()); > > > > > > auto t = py::tuple(cv.numElements()); > > > > > > > > > > > > for (size_t i = 0; i < cv.numElements(); ++i) > > > > > > t[i] = v[i]; > > > > > > > > > > > > return std::move(t); > > > > > > } > > > > > > > > > > > > return py::cast(cv.get<T>()); > > > > > > > > > > > > The type of 't' is py::tuple (replacing 'auto' with 'py::tuple' still > > > > > > produces the same warning), which inherits from py::object. We thus seem > > > > > > to be in the last case described by the above link: > > > > > > > > > > > > There are situations where returning std::move (expr) makes sense, > > > > > > however. The rules for the implicit move require that the selected > > > > > > constructor take an rvalue reference to the returned object's type. > > > > > > Sometimes that isn't the case. For example, when a function returns > > > > > > an object whose type is a class derived from the class type the > > > > > > function returns. In that case, overload resolution is performed a > > > > > > second time, this time treating the object as an lvalue: > > > > > > > > > > > > struct U { }; > > > > > > struct T : U { }; > > > > > > > > > > > > U f() > > > > > > { > > > > > > T t; > > > > > > return std::move (t); > > > > > > } > > > > > > > > > > > > g++-13 produces a warning when compiling that code: > > > > > > > > > > > > move.cpp: In function ‘U f()’: > > > > > > move.cpp:9:26: warning: redundant move in return statement [-Wredundant-move] > > > > > > 9 | return std::move (t); > > > > > > | ~~~~~~~~~~^~~ > > > > > > move.cpp:9:26: note: remove ‘std::move’ call > > > > > > > > > > > > This may also be a false positive of gcc ? > > > > > > > > > > I don't have gcc 13, nor does godbolt.org, but other gcc nor clang > > > > > versions don't complain. > > > > > > > > > > With some testing on godbolt, with and without std::move the end result > > > > > is the same (with -O2) on the compilers I tested. > > > > > > > > > > So... I don't know. The text you pasted seems to suggest that > > > > > std::move() would be needed there, but I don't see a diff (then again, > > > > > my test code is just test code, not the actual py code we have). I'm > > > > > fine either way, but if gcc 13 is not much used yet, maybe we should > > > > > wait a bit? > > > > > > > > > > Also, a bit beside the point, I'm actually a bit surprised that > > > > > > > > > > U f() > > > > > { > > > > > return T(); > > > > > } > > > > > > > > > > works without warnings (even if I add fields to U and T). It's silently > > > > > throwing away the T specific parts, only keeping the U parts. > > > > > > > > I tried this test code: > > > > > > > > -------- > > > > #include <functional> > > > > #include <iostream> > > > > > > > > struct Base { > > > > Base() > > > > { > > > > std::cout << "Base::Base()" << std::endl; > > > > } > > > > > > > > Base(const Base &) > > > > { > > > > std::cout << "Base::Base(const Base &)" << std::endl; > > > > } > > > > > > > > Base(Base &&) > > > > { > > > > std::cout << "Base::Base(Base &&)" << std::endl; > > > > } > > > > }; > > > > > > > > struct Derived : Base { > > > > }; > > > > > > > > Base f() > > > > { > > > > Derived t; > > > > return std::move(t); > > > > } > > > > > > > > Base g() > > > > { > > > > Derived t; > > > > return t; > > > > } > > > > > > > > int main() > > > > { > > > > f(); > > > > g(); > > > > > > > > return 0; > > > > } > > > > -------- > > > > > > > > $ g++-12 -W -Wall -o move2 move2.cpp && ./move2 > > > > Base::Base() > > > > Base::Base(Base &&) > > > > Base::Base() > > > > Base::Base(const Base &) > > > > > > > > $ g++-13 -W -Wall -o move2 move2.cpp && ./move2 > > > > move2.cpp: In function ‘Base f()’: > > > > move2.cpp:27:25: warning: redundant move in return statement [-Wredundant-move] > > > > 27 | return std::move(t); > > > > | ~~~~~~~~~^~~ > > > > move2.cpp:27:25: note: remove ‘std::move’ call > > > > Base::Base() > > > > Base::Base(Base &&) > > > > Base::Base() > > > > Base::Base(Base &&) > > > > > > > > This is annoying. The move seems to be redundant indeed with g++-13, but > > > > dropping it results in the copy constructor being used with g++-12 and > > > > earlier. > > > > > > Worse, I tried > > > > > > #pragma GCC diagnostic push > > > #pragma GCC diagnostic ignored "-Wredundant-move" > > > /* > > > * gcc-13 calls the py::object move constructor even without a > > > * std::move(), making it redundant, but earlier gcc versions > > > * call the copy constructor. Keep the move and silence the > > > * warning. > > > */ > > > return std::move(t); > > > #pragma GCC diagnostic pop > > > > > > which gave me > > > > > > ../../src/py/libcamera/py_helpers.cpp: In function ‘pybind11::object valueOrTuple(const libcamera::ControlValue&)’: > > > ../../src/py/libcamera/py_helpers.cpp:29:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas] > > > #pragma GCC diagnostic ignored "-Wredundant-move" > > > ^~~~~~~~~~~~~~~~~~ > > > > Given the pains here, can we just add -Wno-redundant-move or such for > > 8.5+ ? > > I think it got introduced in 9.1, and I think we can disable it, yes. As > far I understand the redundant moves don't cause any adverse effect. I've sent a patch, see https://patchwork.libcamera.org/patch/18575/ > > > with gcc-8.5.0.
diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp index 79891ab63862..5bedea047e31 100644 --- a/src/py/libcamera/py_helpers.cpp +++ b/src/py/libcamera/py_helpers.cpp @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv) for (size_t i = 0; i < cv.numElements(); ++i) t[i] = v[i]; - return std::move(t); + return t; } return py::cast(cv.get<T>());
gcc-13 warns that the valueOrTuple() function has a redundant std::move() in a return statement: ../../src/py/libcamera/py_helpers.cpp: In instantiation of ‘pybind11::object valueOrTuple(const libcamera::ControlValue&) [with T = bool]’: ../../src/py/libcamera/py_helpers.cpp:38:28: required from here ../../src/py/libcamera/py_helpers.cpp:28:35: error: redundant move in return statement [-Werror=redundant-move] 28 | return std::move(t); Drop it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/py/libcamera/py_helpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63 prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0 prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42 prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391