Hi
2025. 03. 04. 0:04 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
>
> Thank you for the patch.
>
> On Mon, Mar 03, 2025 at 04:14:38PM +0100, Barnabás Pőcze wrote:
>> Use `std::{forward,move}` to forward the arguments, this enables the
>> use of move constructors, likely leading to less code and better runtime.
>> For example, move constructing a libstdc++ `std::shared_ptr` is noticeably
>> less code than copy constructing one.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
>> ---
>> include/libcamera/base/bound_method.h | 26 ++++++++++++++------------
>> 1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
>> index dd3488eeb..47e0a2475 100644
>> --- a/include/libcamera/base/bound_method.h
>> +++ b/include/libcamera/base/bound_method.h
>> @@ -33,8 +33,9 @@ template<typename R, typename... Args>
>> class BoundMethodPack : public BoundMethodPackBase
>> {
>> public:
>> - BoundMethodPack(const Args &... args)
>> - : args_(args...)
>> + template<typename... Ts>
>> + BoundMethodPack(Ts &&...args)
>
> Why is a Ts template parameter needed here, what happens if you use
>
> BoundMethodPack(const Args &... args)
> : args_(std::forward<Args>(args)...)
>
> ?
If `const T&` is used, then the object in the tuple will always be
copy constructed even if the constructor receives a temporary that
could be moved.
Using `BoundMethodPack(Args&&...)` would also not work generally because
that will only accept rvalue references (except when the given type in `Args`
is an lvalue reference).
An alternative would be `BoundMethodPack(Args...)`, but the downside of that
is that it might force unnecessary object construction.
So "forwarding" references are used to avoid the above issues, and to be able
to pass the references through unmodified to the tuple constructor.
Regards,
Barnabás Pőcze
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + : args_(std::forward<Ts>(args)...)
>> {
>> }
>>
>> @@ -51,8 +52,9 @@ template<typename... Args>
>> class BoundMethodPack<void, Args...> : public BoundMethodPackBase
>> {
>> public:
>> - BoundMethodPack(const Args &... args)
>> - : args_(args...)
>> + template<typename... Ts>
>> + BoundMethodPack(Ts &&...args)
>> + : args_(std::forward<Ts>(args)...)
>> {
>> }
>>
>> @@ -136,23 +138,23 @@ public:
>>
>> BoundMethodFunctor(T *obj, Object *object, Func func,
>> ConnectionType type = ConnectionTypeAuto)
>> - : BoundMethodArgs<R, Args...>(obj, object, type), func_(func)
>> + : BoundMethodArgs<R, Args...>(obj, object, type), func_(std::move(func))
>> {
>> }
>>
>> R activate(Args... args, bool deleteMethod = false) override
>> {
>> if (!this->object_)
>> - return func_(args...);
>> + return func_(std::forward<Args>(args)...);
>>
>> - auto pack = std::make_shared<PackType>(args...);
>> + auto pack = std::make_shared<PackType>(std::forward<Args>(args)...);
>> bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
>> return sync ? pack->returnValue() : R();
>> }
>>
>> R invoke(Args... args) override
>> {
>> - return func_(args...);
>> + return func_(std::forward<Args>(args)...);
>> }
>>
>> private:
>> @@ -177,10 +179,10 @@ public:
>> {
>> if (!this->object_) {
>> T *obj = static_cast<T *>(this->obj_);
>> - return (obj->*func_)(args...);
>> + return (obj->*func_)(std::forward<Args>(args)...);
>> }
>>
>> - auto pack = std::make_shared<PackType>(args...);
>> + auto pack = std::make_shared<PackType>(std::forward<Args>(args)...);
>> bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
>> return sync ? pack->returnValue() : R();
>> }
>> @@ -188,7 +190,7 @@ public:
>> R invoke(Args... args) override
>> {
>> T *obj = static_cast<T *>(this->obj_);
>> - return (obj->*func_)(args...);
>> + return (obj->*func_)(std::forward<Args>(args)...);
>> }
>>
>> private:
>> @@ -209,7 +211,7 @@ public:
>>
>> R activate(Args... args, [[maybe_unused]] bool deleteMethod = false) override
>> {
>> - return (*func_)(args...);
>> + return (*func_)(std::forward<Args>(args)...);
>> }
>>
>> R invoke(Args...) override
>