[libcamera-devel] [PATCH 2/2] libcamera: object: Support reference arguments in invokeMethod()

[Jacopo Mondi]

Hi Laurent,

On Fri, Jan 03, 2020 at 01:27:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jan 03, 2020 at 09:57:19AM +0100, Jacopo Mondi wrote:
> > On Fri, Jan 03, 2020 at 01:53:11AM +0200, Laurent Pinchart wrote:
> > > Invoking a method that takes a reference argument with
> > > Object::invokeMethod() results in a compilation error:
> > >
> > > ../test/object-invoke.cpp:131:11: error: no matching member function for call to 'invokeMethod'
> > >                 object_.invokeMethod(&InvokedObject::methodWithReference,
> > >                 ~~~~~~~~^~~~~~~~~~~~
> > > ../include/libcamera/object.h:33:7: note: candidate template ignored: deduced conflicting types for parameter 'Args' (<const int &> vs. <int>)
> > >         void invokeMethod(void (T::*func)(Args...), ConnectionType type, Args... args)
> > >
> > > This is due to the fact that implicit type conversions (from value to
> > > reference in this case) takes place after template argument type
> > > deduction, during overload resolution. A similar issue would occur if
> > > T::func took a long argument and invokeMethod() was called with an in
> > > argument.
> > >
> > > Fix this by specifying to sets of argument types in the invokeMethod()
> > > template, one for the arguments to the invoked method, and one for the
> > > arguments to invokeMethod() itself. The compiler can then first perform
> > > type deduction separately, and implicit conversion in a second step.
> > >
> > > Reported-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  include/libcamera/object.h | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h
> > > index 86e0f7265865..21b70460b516 100644
> > > --- a/include/libcamera/object.h
> > > +++ b/include/libcamera/object.h
> > > @@ -29,13 +29,15 @@ public:
> > >
> > >  	void postMessage(std::unique_ptr<Message> msg);
> > >
> > > -	template<typename T, typename... Args, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>
> > > -	void invokeMethod(void (T::*func)(Args...), ConnectionType type, Args... args)
> > > +	template<typename T, typename... FuncArgs, typename... Args,
> > > +		 typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>
> >
> > I'm not sure I got this to the point I can give any tag, could you
> > help ?
> >
> > How does the compiler knows which templates parameters are part of
> > FuncArgs and which ones of Args, being the two consecutive ?
>
> The first step for the compiler, when encountering a call to
> invokeMethod() such as
>
> 	object_.invokeMethod(&InvokedObject::methodWithReference,
> 			     ConnectionTypeBlocking, 42);
>
> is to perform template argument deduction, as the template arguments are
> not specified explicitly (compare it to ControlValue::get() where we
> write .get<int>() explicitly). With invokeMethod() defined as
>
> 	template<typename T, typename... Args>
> 	void invokeMethod(void (T::*func)(Args...), ConnectionType type,
> 			  Args... args)
>
> (I've left the std::enable_if out for clarity) the compiler thus tries
> to figure out what types T and Args correspond to.
>
> For T, it's easy. The type is mentioned a single time in the
> invokeMethod() signature, as void (T::*func)(Args...). The func
> parameter is equal to &InvokedObject::methodWithReference, which is
> defined as
>
> 	void InvokedObject::methodWithReference(const int &value)
>
> T must thus be equal to InvokedObject.
>
> For Args, the compiler has two sources of information,
> void (T::*func)(Args...) and Args... args. For the former, the func
> parameter being equal to &InvokedObject::methodWithReference, the
> compiler will deduce that Args... is equal to const int &. For the
> latter, the compiler looks at the parameters passed to invokeMethod()
> after the first two, and finds 42, which is of type int. It thus deduces
> that Args... is equal to int. The two deductions don't match,
> compilation fails.
>
> With the new version of invokeMethod() defined as
>
> 	template<typename T, typename... FuncArgs, typename... Args>
> 	void invokeMethod(void (T::*func)(FuncArgs...), ConnectionType type,
> 			  Args... args)
>
> the compiler will deduce T to InvokedObject, FuncArgs... to const int &
> and Args... to int. There's no conflict anymore, this compilation step
> succeeds.
>
> In invokeMethod(), the compiler then encounters
>
> 	void *pack = new typename BoundMemberMethod<T, FuncArgs...>::PackType{ args... };
>
> BoundMemberMethod::PackType is defined as
>
> 	using PackType = std::tuple<typename std::remove_reference<Args>::type...>;
>
> which in this case is
>
> 	std::tuple<typename std::remove_reference<FuncArgs>::type...>
> 	std::tuple<typename std::remove_reference<const int &>::type>
> 	std::tuple<const int>
>
> resulting in
>
> 	void *pack = new std::tuple<const int>{ args... };
>
> args is an int, so this succeeds. Note that if args was an int & this
> would succeed as well, as the compiler would perform implicit conversion
> from reference to value. If the types were incompatible, for instance if
> the test called
>
> 	object_.invokeMethod(&InvokedObject::methodWithReference,
> 			     ConnectionTypeBlocking, "foo");
>
> the compiler would still complain that a const char * can't be
> implicitly converted to an int. This patch doesn't remove all compiler
> safety checks, it only relaxes the constraints on template argument
> deduction.
>

Thanks, very clear and informative!
Please have my
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

and saved in my "how to deal with C++ preserving your remaining
sanity" notes

Thanks
   j

> > > +	void invokeMethod(void (T::*func)(FuncArgs...), ConnectionType type,
> > > +			  Args... args)
> > >  	{
> > >  		T *obj = static_cast<T *>(this);
> > >  		BoundMethodBase *method =
> > > -			new BoundMemberMethod<T, Args...>(obj, this, func, type);
> > > -		void *pack = new typename BoundMemberMethod<T, Args...>::PackType{ args... };
> > > +			new BoundMemberMethod<T, FuncArgs...>(obj, this, func, type);
> > > +		void *pack = new typename BoundMemberMethod<T, FuncArgs...>::PackType{ args... };
> > >
> > >  		method->activatePack(pack, true);
> > >  	}
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200103/cf433df6/attachment.sig>