[libcamera-devel] [PATCH] libcamera: object: Avoid argument copies in invokeMethod()

[Umang Jain]

Hi Laurent,

Thank you for the patch

On 11/19/21 5:15 AM, Laurent Pinchart wrote:
> Template argument deduction results in the lvalue and lvalue reference
> arguments to the invokeMethod() function causing deduction of the Args
> template type to a non-reference type. This results in the argument
> being passed by value and copied.


I think this is a documented behavior (and by documented, I mean 
intentional...)

          *
          * Arguments \a args passed by value or reference are copied, 
while pointers
          * are passed untouched. The caller shall ensure that any 
pointer argument
          * remains valid until the method is invoked.

Should the documented behavior be tweaked as well?

>
> Fix this by using a cv-unqualified rvalue reference parameter type. The
> type is then deduced to an lvalue reference when the argument is an


So does this mean, references can be passed from one thread to another 
in ::invokeMethod() ? Instead of copy-by-value behavior as before? Are 
there any caveats for doing so?


> lvalue or lvalue reference, due to a combination of the special template
> argument deduction rule for rvalue reference parameter types:
>
>    If P is an rvalue reference to a cv-unqualified template parameter
>    (so-called forwarding reference), and the corresponding function call
>    argument is an lvalue, the type lvalue reference to A is used in place
>    of A for deduction.
>
>    (https://en.cppreference.com/w/cpp/language/template_argument_deduction)
>
> and the reference collapsing rule
> (https://en.cppreference.com/w/cpp/language/reference#Reference_collapsing).


As long as we are OK with passing references(lvalue or rvalue) across 
threads (that's my understanding of use-case of ::invokeMethod()) the 
reference collapsing done here makes sense.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   include/libcamera/base/object.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h
> index 5c385ab4b140..ae2a275caf68 100644
> --- a/include/libcamera/base/object.h
> +++ b/include/libcamera/base/object.h
> @@ -34,7 +34,7 @@ public:
>   	template<typename T, typename R, typename... FuncArgs, typename... Args,
>   		 typename std::enable_if_t<std::is_base_of<Object, T>::value> * = nullptr>
>   	R invokeMethod(R (T::*func)(FuncArgs...), ConnectionType type,
> -		       Args... args)
> +		       Args&&... args)
>   	{
>   		T *obj = static_cast<T *>(this);
>   		auto *method = new BoundMethodMember<T, R, FuncArgs...>(obj, this, func, type);
>
> base-commit: d9a2a1f703273a28b001dee40fddd378bba7a1b6