| Message ID | 20260508092816.119642-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Fri, May 08, 2026 at 11:28:16AM +0200, Barnabás Pőcze wrote: > This implements the same kind of change for pack-based invoking as 9c86a405b72a > ("libcamera: base: bound_method: Forward arguments when possible") did for the > other cases. > > At the moment a single argument pack is used just once, hence it is safe > to forward the arguments from the pack, allowing moving to take place > instead of copying. So do that. This seems useful for Object::invokeMethod(). I'm a bit concerned that we're slowly moving towards a situation where the Signal class would compile with rvalue reference arguments though. That would lead to undefined behaviour if the signal gets connected to multiple slots. Maybe rvalue references for signals are desirable if we enforce that the signal gets connected once and once only, but that's a separate discussion. For the time being, my mind would find peace if you could add a static assert to the Signal class to avoid this issue. > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/bound_method.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h > index cb642e0a9b..9b42a8a1e8 100644 > --- a/include/libcamera/base/bound_method.h > +++ b/include/libcamera/base/bound_method.h > @@ -97,9 +97,9 @@ private: > [[maybe_unused]] auto *args = static_cast<PackType *>(pack); > > if constexpr (!std::is_void_v<R>) > - args->ret_ = invoke(std::get<I>(args->args_)...); > + args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...); > else > - invoke(std::get<I>(args->args_)...); > + invoke(std::forward<Args>(std::get<I>(args->args_))...); > } > > public:
Quoting Laurent Pinchart (2026-05-08 13:04:17) > Hi Barnabás, > > Thank you for the patch. > > On Fri, May 08, 2026 at 11:28:16AM +0200, Barnabás Pőcze wrote: > > This implements the same kind of change for pack-based invoking as 9c86a405b72a > > ("libcamera: base: bound_method: Forward arguments when possible") did for the > > other cases. > > > > At the moment a single argument pack is used just once, hence it is safe > > to forward the arguments from the pack, allowing moving to take place > > instead of copying. So do that. > > This seems useful for Object::invokeMethod(). I'm a bit concerned that > we're slowly moving towards a situation where the Signal class would > compile with rvalue reference arguments though. That would lead to > undefined behaviour if the signal gets connected to multiple slots. > > Maybe rvalue references for signals are desirable if we enforce that the > signal gets connected once and once only, but that's a separate > discussion. For the time being, my mind would find peace if you could > add a static assert to the Signal class to avoid this issue. Well, as that's handled: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > include/libcamera/base/bound_method.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h > > index cb642e0a9b..9b42a8a1e8 100644 > > --- a/include/libcamera/base/bound_method.h > > +++ b/include/libcamera/base/bound_method.h > > @@ -97,9 +97,9 @@ private: > > [[maybe_unused]] auto *args = static_cast<PackType *>(pack); > > > > if constexpr (!std::is_void_v<R>) > > - args->ret_ = invoke(std::get<I>(args->args_)...); > > + args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...); > > else > > - invoke(std::get<I>(args->args_)...); > > + invoke(std::forward<Args>(std::get<I>(args->args_))...); > > } > > > > public: > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h index cb642e0a9b..9b42a8a1e8 100644 --- a/include/libcamera/base/bound_method.h +++ b/include/libcamera/base/bound_method.h @@ -97,9 +97,9 @@ private: [[maybe_unused]] auto *args = static_cast<PackType *>(pack); if constexpr (!std::is_void_v<R>) - args->ret_ = invoke(std::get<I>(args->args_)...); + args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...); else - invoke(std::get<I>(args->args_)...); + invoke(std::forward<Args>(std::get<I>(args->args_))...); } public:
This implements the same kind of change for pack-based invoking as 9c86a405b72a ("libcamera: base: bound_method: Forward arguments when possible") did for the other cases. At the moment a single argument pack is used just once, hence it is safe to forward the arguments from the pack, allowing moving to take place instead of copying. So do that. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/base/bound_method.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)