[libcamera-devel] [PATCH] py: Drop redundant std::move()
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Fri Apr 28 17:03:18 CEST 2023
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);
Tomi
More information about the libcamera-devel
mailing list