| Message ID | 20260508221033.2417134-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 05. 09. 0:10 keltezéssel, Laurent Pinchart írta: > Now that libcamera uses C++20, we can use std::apply to replace the > custom implementation. As far as I'm aware `std::apply` is C++17. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/bound_method.h | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h > index 91fe8b8cb969..ad5374f70b18 100644 > --- a/include/libcamera/base/bound_method.h > +++ b/include/libcamera/base/bound_method.h > @@ -91,15 +91,14 @@ public: > using PackType = BoundMethodPack<R, Args...>; > > private: > - template<std::size_t... I> > - void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>) > + void invokePack(PackType *args) > { > - [[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_ = std::apply(&BoundMethodArgs::invoke, > + std::tuple_cat(std::forward_as_tuple(this), args->args_)); > else > - invoke(std::get<I>(args->args_)...); > + std::apply(&BoundMethodArgs::invoke, > + std::tuple_cat(std::forward_as_tuple(this), args->args_)); I think `tuple_cat` preserves the types, so since `args_` contains no references, this will make another copy of each argument. If `std::move(args->args_)` is used, that should only move construct the new elements, but even that seems like an unnecessary overhead to me. > } > > public: > @@ -108,7 +107,7 @@ public: > > void invokePack(BoundMethodPackBase *pack) override > { > - invokePack(pack, std::make_index_sequence<sizeof...(Args)>{}); > + invokePack(static_cast<PackType *>(pack)); If we want to use `std::apply`, I'd inline things here: std::apply([&](auto&&... args /* or Args&&... args */) { if constexpr (!std::is_void_v<R>) args->ret_ = invoke(std::forward<decltype(args)>(args)...); else invoke(std::forward<decltype(args)>(args)...); }, std::move(args->args_)); > } > > virtual R activate(Args... args, bool deleteMethod = false) = 0; > > base-commit: 2ca75f012fa12621ed70e333e635d173e92593e5
On Mon, May 11, 2026 at 10:58:27AM +0200, Barnabás Pőcze wrote: > 2026. 05. 09. 0:10 keltezéssel, Laurent Pinchart írta: > > Now that libcamera uses C++20, we can use std::apply to replace the > > custom implementation. > > As far as I'm aware `std::apply` is C++17. It is. I've had this patch in my tree for a long time, and the commit message stated C++17. I replaced it with C++20 before sending it, as we've switched to C++20. I don't mind writing C++17. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/bound_method.h | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h > > index 91fe8b8cb969..ad5374f70b18 100644 > > --- a/include/libcamera/base/bound_method.h > > +++ b/include/libcamera/base/bound_method.h > > @@ -91,15 +91,14 @@ public: > > using PackType = BoundMethodPack<R, Args...>; > > > > private: > > - template<std::size_t... I> > > - void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>) > > + void invokePack(PackType *args) > > { > > - [[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_ = std::apply(&BoundMethodArgs::invoke, > > + std::tuple_cat(std::forward_as_tuple(this), args->args_)); > > else > > - invoke(std::get<I>(args->args_)...); > > + std::apply(&BoundMethodArgs::invoke, > > + std::tuple_cat(std::forward_as_tuple(this), args->args_)); > > I think `tuple_cat` preserves the types, so since `args_` contains no references, this will > make another copy of each argument. If `std::move(args->args_)` is used, that should only > move construct the new elements, but even that seems like an unnecessary overhead to me. > > > } > > > > public: > > @@ -108,7 +107,7 @@ public: > > > > void invokePack(BoundMethodPackBase *pack) override > > { > > - invokePack(pack, std::make_index_sequence<sizeof...(Args)>{}); > > + invokePack(static_cast<PackType *>(pack)); > > If we want to use `std::apply`, I'd inline things here: Ah good point, the private overload of invokePack() isn't needed any more. I'll send a v2. > std::apply([&](auto&&... args /* or Args&&... args */) { > if constexpr (!std::is_void_v<R>) > args->ret_ = invoke(std::forward<decltype(args)>(args)...); > else > invoke(std::forward<decltype(args)>(args)...); > }, std::move(args->args_)); This means the pack can be used once only. It should be fine, as every bound method connected to a signal gets its own pack. > > } > > > > virtual R activate(Args... args, bool deleteMethod = false) = 0; > > > > base-commit: 2ca75f012fa12621ed70e333e635d173e92593e5
2026. 05. 11. 12:20 keltezéssel, Laurent Pinchart írta: > On Mon, May 11, 2026 at 10:58:27AM +0200, Barnabás Pőcze wrote: >> 2026. 05. 09. 0:10 keltezéssel, Laurent Pinchart írta: >>> Now that libcamera uses C++20, we can use std::apply to replace the >>> custom implementation. >> >> As far as I'm aware `std::apply` is C++17. > > It is. I've had this patch in my tree for a long time, and the commit > message stated C++17. I replaced it with C++20 before sending it, as > we've switched to C++20. I don't mind writing C++17. > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> include/libcamera/base/bound_method.h | 13 ++++++------- >>> 1 file changed, 6 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h >>> index 91fe8b8cb969..ad5374f70b18 100644 >>> --- a/include/libcamera/base/bound_method.h >>> +++ b/include/libcamera/base/bound_method.h >>> @@ -91,15 +91,14 @@ public: >>> using PackType = BoundMethodPack<R, Args...>; >>> >>> private: >>> - template<std::size_t... I> >>> - void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>) >>> + void invokePack(PackType *args) >>> { >>> - [[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_ = std::apply(&BoundMethodArgs::invoke, >>> + std::tuple_cat(std::forward_as_tuple(this), args->args_)); >>> else >>> - invoke(std::get<I>(args->args_)...); >>> + std::apply(&BoundMethodArgs::invoke, >>> + std::tuple_cat(std::forward_as_tuple(this), args->args_)); >> >> I think `tuple_cat` preserves the types, so since `args_` contains no references, this will >> make another copy of each argument. If `std::move(args->args_)` is used, that should only >> move construct the new elements, but even that seems like an unnecessary overhead to me. >> >>> } >>> >>> public: >>> @@ -108,7 +107,7 @@ public: >>> >>> void invokePack(BoundMethodPackBase *pack) override >>> { >>> - invokePack(pack, std::make_index_sequence<sizeof...(Args)>{}); >>> + invokePack(static_cast<PackType *>(pack)); >> >> If we want to use `std::apply`, I'd inline things here: > > Ah good point, the private overload of invokePack() isn't needed any > more. I'll send a v2. > >> std::apply([&](auto&&... args /* or Args&&... args */) { >> if constexpr (!std::is_void_v<R>) >> args->ret_ = invoke(std::forward<decltype(args)>(args)...); >> else >> invoke(std::forward<decltype(args)>(args)...); >> }, std::move(args->args_)); > > This means the pack can be used once only. It should be fine, as every > bound method connected to a signal gets its own pack. Yes, but I believe we have recently agreed that is fine? At least I have already merged https://patchwork.libcamera.org/patch/26688/ which does effectively the same "move". > >>> } >>> >>> virtual R activate(Args... args, bool deleteMethod = false) = 0; >>> >>> base-commit: 2ca75f012fa12621ed70e333e635d173e92593e5 >
On Mon, May 11, 2026 at 12:24:19PM +0200, Barnabás Pőcze wrote: > 2026. 05. 11. 12:20 keltezéssel, Laurent Pinchart írta: > > On Mon, May 11, 2026 at 10:58:27AM +0200, Barnabás Pőcze wrote: > >> 2026. 05. 09. 0:10 keltezéssel, Laurent Pinchart írta: > >>> Now that libcamera uses C++20, we can use std::apply to replace the > >>> custom implementation. > >> > >> As far as I'm aware `std::apply` is C++17. > > > > It is. I've had this patch in my tree for a long time, and the commit > > message stated C++17. I replaced it with C++20 before sending it, as > > we've switched to C++20. I don't mind writing C++17. > > > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> include/libcamera/base/bound_method.h | 13 ++++++------- > >>> 1 file changed, 6 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h > >>> index 91fe8b8cb969..ad5374f70b18 100644 > >>> --- a/include/libcamera/base/bound_method.h > >>> +++ b/include/libcamera/base/bound_method.h > >>> @@ -91,15 +91,14 @@ public: > >>> using PackType = BoundMethodPack<R, Args...>; > >>> > >>> private: > >>> - template<std::size_t... I> > >>> - void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>) > >>> + void invokePack(PackType *args) > >>> { > >>> - [[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_ = std::apply(&BoundMethodArgs::invoke, > >>> + std::tuple_cat(std::forward_as_tuple(this), args->args_)); > >>> else > >>> - invoke(std::get<I>(args->args_)...); > >>> + std::apply(&BoundMethodArgs::invoke, > >>> + std::tuple_cat(std::forward_as_tuple(this), args->args_)); > >> > >> I think `tuple_cat` preserves the types, so since `args_` contains no references, this will > >> make another copy of each argument. If `std::move(args->args_)` is used, that should only > >> move construct the new elements, but even that seems like an unnecessary overhead to me. > >> > >>> } > >>> > >>> public: > >>> @@ -108,7 +107,7 @@ public: > >>> > >>> void invokePack(BoundMethodPackBase *pack) override > >>> { > >>> - invokePack(pack, std::make_index_sequence<sizeof...(Args)>{}); > >>> + invokePack(static_cast<PackType *>(pack)); > >> > >> If we want to use `std::apply`, I'd inline things here: > > > > Ah good point, the private overload of invokePack() isn't needed any > > more. I'll send a v2. > > > >> std::apply([&](auto&&... args /* or Args&&... args */) { > >> if constexpr (!std::is_void_v<R>) > >> args->ret_ = invoke(std::forward<decltype(args)>(args)...); > >> else > >> invoke(std::forward<decltype(args)>(args)...); > >> }, std::move(args->args_)); > > > > This means the pack can be used once only. It should be fine, as every > > bound method connected to a signal gets its own pack. > > Yes, but I believe we have recently agreed that is fine? > At least I have already merged https://patchwork.libcamera.org/patch/26688/ > which does effectively the same "move". Yes, I was just thinking out loud :-) > >>> } > >>> > >>> virtual R activate(Args... args, bool deleteMethod = false) = 0; > >>> > >>> base-commit: 2ca75f012fa12621ed70e333e635d173e92593e5
diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h index 91fe8b8cb969..ad5374f70b18 100644 --- a/include/libcamera/base/bound_method.h +++ b/include/libcamera/base/bound_method.h @@ -91,15 +91,14 @@ public: using PackType = BoundMethodPack<R, Args...>; private: - template<std::size_t... I> - void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>) + void invokePack(PackType *args) { - [[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_ = std::apply(&BoundMethodArgs::invoke, + std::tuple_cat(std::forward_as_tuple(this), args->args_)); else - invoke(std::get<I>(args->args_)...); + std::apply(&BoundMethodArgs::invoke, + std::tuple_cat(std::forward_as_tuple(this), args->args_)); } public: @@ -108,7 +107,7 @@ public: void invokePack(BoundMethodPackBase *pack) override { - invokePack(pack, std::make_index_sequence<sizeof...(Args)>{}); + invokePack(static_cast<PackType *>(pack)); } virtual R activate(Args... args, bool deleteMethod = false) = 0;
Now that libcamera uses C++20, we can use std::apply to replace the custom implementation. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/bound_method.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) base-commit: 2ca75f012fa12621ed70e333e635d173e92593e5