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

[Tomi Valkeinen]

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 at ideasonboard.com>

  Tomi