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

[Laurent Pinchart]

Hi Umang,

On Tue, Aug 31, 2021 at 11:13:12AM +0530, Umang Jain wrote:
> 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>
> > ---
> >   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>
> 
> 
> As you have explained to me the workaround of T= R on IRC, that 
> std::enable_if works on function template argument typename rather on 
> class template argument, do you mind capturing this as a comment here or 
> in the commit message while pushing?

I'll add this to the commit messge:

SFINAE can only depend on the function template parameters, not the
parameters of the class template in which the function is defined:

"Only the failures in the types and expressions in the immediate context
of the function type or its template parameter types are SFINAE errors."

We thus can't use the type R in an std::enable_if expression for the
invokePack() function. To work around this, we have to add a type T to
the function template definition, which defaults to R, and use T with
std::enable_if.

> > +	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>

-- 
Regards,

Laurent Pinchart