[PATCH v1] libcamera: base: bound_method: Forward arguments when possible

[Laurent Pinchart]

Hi Barnabás,

Thank you for the patch.

On Mon, Mar 03, 2025 at 04:14:38PM +0100, Barnabás Pőcze wrote:
> Use `std::{forward,move}` to forward the arguments, this enables the
> use of move constructors, likely leading to less code and better runtime.
> For example, move constructing a libstdc++ `std::shared_ptr` is noticeably
> less code than copy constructing one.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  include/libcamera/base/bound_method.h | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index dd3488eeb..47e0a2475 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -33,8 +33,9 @@ template<typename R, typename... Args>
>  class BoundMethodPack : public BoundMethodPackBase
>  {
>  public:
> -	BoundMethodPack(const Args &... args)
> -		: args_(args...)
> +	template<typename... Ts>
> +	BoundMethodPack(Ts &&...args)

Why is a Ts template parameter needed here, what happens if you use

	BoundMethodPack(const Args &... args)
		: args_(std::forward<Args>(args)...)

?

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

> +		: args_(std::forward<Ts>(args)...)
>  	{
>  	}
>  
> @@ -51,8 +52,9 @@ template<typename... Args>
>  class BoundMethodPack<void, Args...> : public BoundMethodPackBase
>  {
>  public:
> -	BoundMethodPack(const Args &... args)
> -		: args_(args...)
> +	template<typename... Ts>
> +	BoundMethodPack(Ts &&...args)
> +		: args_(std::forward<Ts>(args)...)
>  	{
>  	}
>  
> @@ -136,23 +138,23 @@ public:
>  
>  	BoundMethodFunctor(T *obj, Object *object, Func func,
>  			   ConnectionType type = ConnectionTypeAuto)
> -		: BoundMethodArgs<R, Args...>(obj, object, type), func_(func)
> +		: BoundMethodArgs<R, Args...>(obj, object, type), func_(std::move(func))
>  	{
>  	}
>  
>  	R activate(Args... args, bool deleteMethod = false) override
>  	{
>  		if (!this->object_)
> -			return func_(args...);
> +			return func_(std::forward<Args>(args)...);
>  
> -		auto pack = std::make_shared<PackType>(args...);
> +		auto pack = std::make_shared<PackType>(std::forward<Args>(args)...);
>  		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
>  		return sync ? pack->returnValue() : R();
>  	}
>  
>  	R invoke(Args... args) override
>  	{
> -		return func_(args...);
> +		return func_(std::forward<Args>(args)...);
>  	}
>  
>  private:
> @@ -177,10 +179,10 @@ public:
>  	{
>  		if (!this->object_) {
>  			T *obj = static_cast<T *>(this->obj_);
> -			return (obj->*func_)(args...);
> +			return (obj->*func_)(std::forward<Args>(args)...);
>  		}
>  
> -		auto pack = std::make_shared<PackType>(args...);
> +		auto pack = std::make_shared<PackType>(std::forward<Args>(args)...);
>  		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
>  		return sync ? pack->returnValue() : R();
>  	}
> @@ -188,7 +190,7 @@ public:
>  	R invoke(Args... args) override
>  	{
>  		T *obj = static_cast<T *>(this->obj_);
> -		return (obj->*func_)(args...);
> +		return (obj->*func_)(std::forward<Args>(args)...);
>  	}
>  
>  private:
> @@ -209,7 +211,7 @@ public:
>  
>  	R activate(Args... args, [[maybe_unused]] bool deleteMethod = false) override
>  	{
> -		return (*func_)(args...);
> +		return (*func_)(std::forward<Args>(args)...);
>  	}
>  
>  	R invoke(Args...) override

-- 
Regards,

Laurent Pinchart