[v1] libcamera: base: bound_method: Move return value

23598 diff mbox series

Instead of copying, just move the returned value when the call is made
through an argument pack. This enables, e.g. `Object::invokeMethod()`
to be usable with functions returning types, such as`std::unique_ptr`,
that have no copy ctor/assignment. Since there are no other users of
the argument pack object, this is safe to do. Reference return types
are not supported, so a simple `std::move()` is sufficient.

Link: https://bugs.libcamera.org/show_bug.cgi?id=273#c1
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/base/bound_method.h | 17 ++++++-----------
 test/object-invoke.cpp                | 16 ++++++++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

Hi Barnabás,

Thank you for the patch.

On Wed, Jun 18, 2025 at 04:45:36PM +0200, Barnabás Pőcze wrote:
> Instead of copying, just move the returned value when the call is made
> through an argument pack. This enables, e.g. `Object::invokeMethod()`
> to be usable with functions returning types, such as`std::unique_ptr`,
> that have no copy ctor/assignment. Since there are no other users of
> the argument pack object, this is safe to do. Reference return types
> are not supported, so a simple `std::move()` is sufficient.
> 
> Link: https://bugs.libcamera.org/show_bug.cgi?id=273#c1

We usually use Bug:.

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/base/bound_method.h | 17 ++++++-----------
>  test/object-invoke.cpp                | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index 507c320d3..8b433a0da 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -38,11 +38,6 @@ public:
>  	{
>  	}
>  
> -	R returnValue()
> -	{
> -		return ret_;
> -	}
> -
>  	std::tuple<typename std::remove_reference_t<Args>...> args_;
>  	R ret_;
>  };
> @@ -56,10 +51,6 @@ public:
>  	{
>  	}
>  
> -	void returnValue()
> -	{
> -	}
> -
>  	std::tuple<typename std::remove_reference_t<Args>...> args_;
>  };
>  
> @@ -141,7 +132,9 @@ public:
>  
>  		auto pack = std::make_shared<PackType>(args...);
>  		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
> -		return sync ? pack->returnValue() : R();
> +
> +		if constexpr (!std::is_void_v<R>)
> +			return sync ? std::move(pack->ret_) : R();
>  	}
>  
>  	R invoke(Args... args) override
> @@ -176,7 +169,9 @@ public:
>  
>  		auto pack = std::make_shared<PackType>(args...);
>  		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
> -		return sync ? pack->returnValue() : R();
> +
> +		if constexpr (!std::is_void_v<R>)
> +			return sync ? std::move(pack->ret_) : R();
>  	}
>  
>  	R invoke(Args... args) override
> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp
> index def1e61e4..d309cff15 100644
> --- a/test/object-invoke.cpp
> +++ b/test/object-invoke.cpp
> @@ -58,6 +58,19 @@ public:
>  		return 42;
>  	}
>  
> +	struct MoveOnly {
> +		MoveOnly() = default;
> +		MoveOnly(const MoveOnly &) = delete;
> +		MoveOnly &operator=(const MoveOnly &) = delete;
> +		MoveOnly(MoveOnly &&) = default;
> +		MoveOnly &operator=(MoveOnly &&) = default;
> +	};
> +
> +	MoveOnly methodWithReturnMoveOnly()
> +	{
> +		return {};
> +	}
> +
>  private:
>  	Status status_;
>  	int value_;
> @@ -186,6 +199,9 @@ protected:
>  			return TestFail;
>  		}
>  
> +		/* Test invoking a method that returns type with no copy ctor/assignment. */
> +		object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly, ConnectionTypeBlocking);

You can wrap this line.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  		return TestPass;
>  	}
>