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

[Laurent Pinchart]

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 ?

> > Drop it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/py/libcamera/py_helpers.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp
> > index 79891ab63862..5bedea047e31 100644
> > --- a/src/py/libcamera/py_helpers.cpp
> > +++ b/src/py/libcamera/py_helpers.cpp
> > @@ -25,7 +25,7 @@ static py::object valueOrTuple(const ControlValue &cv)
> >                 for (size_t i = 0; i < cv.numElements(); ++i)
> >                         t[i] = v[i];
> >  
> > -               return std::move(t);
> > +               return t;
> >         }
> >  
> >         return py::cast(cv.get<T>());
> > 
> > base-commit: 13986d6ce3ab64c44a8f086ef8942f56bbedff63
> > prerequisite-patch-id: f6a3b225240a9069104d326be29ae2451ba8e9f0
> > prerequisite-patch-id: 8d95f21764dd480d4197b573e213721a7b6dae42
> > prerequisite-patch-id: 7eff091a4898b00438bac219873379769811c391

-- 
Regards,

Laurent Pinchart