[libcamera-devel] [PATCH] py: Drop redundant std::move()

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart