| Message ID | 20250303151438.732916-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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)...) ? Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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@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@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 >
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) + : 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
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@ideasonboard.com> --- include/libcamera/base/bound_method.h | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)