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

[Tomi Valkeinen]

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