[libcamera-devel] [PATCH] py: Drop redundant std::move()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon May 1 05:16:59 CEST 2023
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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list