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

[Tomi Valkeinen]

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