[libcamera-devel] [PATCH v1 2/6] libcamera: base: bound_method: Remove BoundMethodMember specialization

[Umang Jain]

Hi Laurent,

On 8/27/21 8:08 AM, Laurent Pinchart wrote:
> The BoundMethodMember specialization for the void return type is only
> needed to avoid accessing the ret_ member variable that is lacking from
> the corresponding BoundMethodPack specialization. By adding a
> BoundMethodPack::returnValue() function to read the member variable, we
> can remove the complete BoundMethodMember specialization.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   include/libcamera/base/bound_method.h | 46 ++++++---------------------
>   1 file changed, 10 insertions(+), 36 deletions(-)


Ok Let's see, my review attempt #3 to understand these patches. There is 
quite a level of inheritance of classes going on here.. so I hope I 
don't get lost.

>
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index 282f9b58ab60..9c212f1ecc3a 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -38,6 +38,11 @@ public:
>   	{
>   	}
>   
> +	R returnValue()
> +	{
> +		return ret_;
> +	}
> +
Make sense.
>   	std::tuple<typename std::remove_reference_t<Args>...> args_;
>   	R ret_;
>   };
> @@ -51,6 +56,10 @@ public:
>   	{
>   	}
>   
> +	void returnValue()
> +	{
> +	}
> +


Makes sense

>   	std::tuple<typename std::remove_reference_t<Args>...> args_;
>   };
>   
> @@ -160,7 +169,7 @@ public:
>   
>   		auto pack = std::make_shared<PackType>(args...);
>   		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
> -		return sync ? pack->ret_ : R();
> +		return sync ? pack->returnValue() : R();


Yep

>   	}
>   
>   	R invoke(Args... args) override
> @@ -173,41 +182,6 @@ private:
>   	R (T::*func_)(Args...);
>   };
>   
> -template<typename T, typename... Args>
> -class BoundMethodMember<T, void, Args...> : public BoundMethodArgs<void, Args...>

Ah nice. This is nuked.

I was going to write that we probably need to nuke BoundMethodArgs<void, 
Args...> as no class inherits that now.

...But I already see that happening in 3/6, so slowly I am getting the 
essence of it yay!

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

> -{
> -public:
> -	using PackType = typename BoundMethodArgs<void, Args...>::PackType;
> -
> -	BoundMethodMember(T *obj, Object *object, void (T::*func)(Args...),
> -			  ConnectionType type = ConnectionTypeAuto)
> -		: BoundMethodArgs<void, Args...>(obj, object, type), func_(func)
> -	{
> -	}
> -
> -	bool match(void (T::*func)(Args...)) const { return func == func_; }
> -
> -	void activate(Args... args, bool deleteMethod = false) override
> -	{
> -		if (!this->object_) {
> -			T *obj = static_cast<T *>(this->obj_);
> -			return (obj->*func_)(args...);
> -		}
> -
> -		auto pack = std::make_shared<PackType>(args...);
> -		BoundMethodBase::activatePack(pack, deleteMethod);
> -	}
> -
> -	void invoke(Args... args) override
> -	{
> -		T *obj = static_cast<T *>(this->obj_);
> -		return (obj->*func_)(args...);
> -	}
> -
> -private:
> -	void (T::*func_)(Args...);
> -};
> -
>   template<typename R, typename... Args>
>   class BoundMethodStatic : public BoundMethodArgs<R, Args...>
>   {