[libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method: Remove BoundMethodArgs specialization

[Umang Jain]

Hi Laurent,

Thanks for the patch

On 8/27/21 8:08 AM, Laurent Pinchart wrote:
> The BoundMethodArgs specialization for the void return type is only
> needed to avoid accessing the ret_ member variable that is lacking from
> the corresponding BoundMethodPack specialization. As the member variable
> is only accessed in a single function, instead of specializing the whole
> class we can use SFINAE to select between two different implementations
> of the function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Looks good overall to me, however the diff generated is confusing? I'll 
apply it locally and re-read bound_method.h for my understanding.

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> ---
>   include/libcamera/base/bound_method.h | 34 +++++++--------------------
>   1 file changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index 9c212f1ecc3a..76ce8017e721 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -98,35 +98,17 @@ public:
>   	using PackType = BoundMethodPack<R, Args...>;
>   
>   private:
> -	template<std::size_t... I>
> -	void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> +	template<std::size_t... I, typename T = R>
> +	std::enable_if_t<!std::is_void<T>::value, void>
> +	invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
>   	{
>   		PackType *args = static_cast<PackType *>(pack);
>   		args->ret_ = invoke(std::get<I>(args->args_)...);
>   	}
>   
> -public:
> -	BoundMethodArgs(void *obj, Object *object, ConnectionType type)
> -		: BoundMethodBase(obj, object, type) {}
> -
> -	void invokePack(BoundMethodPackBase *pack) override
> -	{
> -		invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
> -	}
> -
> -	virtual R activate(Args... args, bool deleteMethod = false) = 0;
> -	virtual R invoke(Args... args) = 0;
> -};
> -
> -template<typename... Args>
> -class BoundMethodArgs<void, Args...> : public BoundMethodBase
> -{
> -public:
> -	using PackType = BoundMethodPack<void, Args...>;
> -
> -private:
> -	template<std::size_t... I>
> -	void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> +	template<std::size_t... I, typename T = R>
> +	std::enable_if_t<std::is_void<T>::value, void>
> +	invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
>   	{
>   		/* args is effectively unused when the sequence I is empty. */
>   		PackType *args [[gnu::unused]] = static_cast<PackType *>(pack);
> @@ -142,8 +124,8 @@ public:
>   		invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
>   	}
>   
> -	virtual void activate(Args... args, bool deleteMethod = false) = 0;
> -	virtual void invoke(Args... args) = 0;
> +	virtual R activate(Args... args, bool deleteMethod = false) = 0;
> +	virtual R invoke(Args... args) = 0;
>   };
>   
>   template<typename T, typename R, typename... Args>