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

[Tomi Valkeinen]

On 28/04/2023 18:10, Tomi Valkeinen wrote:
> 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
> 

Or this also seems to do the trick:

return static_cast<Base&&>(std::move(t));

  Tomi