[libcamera-devel,v1,3/6] libcamera: base: bound_method: Remove BoundMethodArgs specialization
diff mbox series

Message ID 20210827023829.5871-4-laurent.pinchart@ideasonboard.com
State Accepted
Commit c4e2b00d51f150632b203f81866604168d2ac1ff
Headers show
Series
  • libcamera: Drop emitter object pointer from signal arguments
Related show

Commit Message

Laurent Pinchart Aug. 27, 2021, 2:38 a.m. UTC 
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@ideasonboard.com>
---
 include/libcamera/base/bound_method.h | 34 +++++++--------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

Comments

Umang Jain Aug. 30, 2021, 4:36 p.m. UTC | #1 
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@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@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>
Umang Jain Aug. 31, 2021, 5:43 a.m. UTC | #2 
Hi Laurent,

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@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?

> +	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>
Laurent Pinchart Aug. 31, 2021, 9:28 a.m. UTC | #3 
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@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>

Patch
diff mbox series 

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>