[v2] libcamera: bound_method: Use std::apply
diff mbox series

Message ID 20260511111121.3042550-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [v2] libcamera: bound_method: Use std::apply
Related show

Commit Message

Laurent Pinchart May 11, 2026, 11:11 a.m. UTC 
Now that libcamera uses C++17, we can use std::apply to replace the
custom implementation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Drop private invokePack() overload
- Use lambda function and std::move() to avoid copy
---
 include/libcamera/base/bound_method.h | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)


base-commit: 500c2320619a47bd01d1ffe69ca4cc6eae6e00e8

Comments

Barnabás Pőcze May 11, 2026, 12:20 p.m. UTC | #1 
2026. 05. 11. 13:11 keltezéssel, Laurent Pinchart írta:
> Now that libcamera uses C++17, we can use std::apply to replace the
> custom implementation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Drop private invokePack() overload
> - Use lambda function and std::move() to avoid copy
> ---

Looks ok to me.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   include/libcamera/base/bound_method.h | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index 9b42a8a1e8ca..f11159e8a0a4 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -90,25 +90,19 @@ class BoundMethodArgs : public BoundMethodBase
>   public:
>   	using PackType = BoundMethodPack<R, Args...>;
>   
> -private:
> -	template<std::size_t... I>
> -	void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> -	{
> -		[[maybe_unused]] auto *args = static_cast<PackType *>(pack);
> -
> -		if constexpr (!std::is_void_v<R>)
> -			args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...);
> -		else
> -			invoke(std::forward<Args>(std::get<I>(args->args_))...);
> -	}
> -
> -public:
>   	BoundMethodArgs(void *obj, Object *object, ConnectionType type)
>   		: BoundMethodBase(obj, object, type) {}
>   
>   	void invokePack(BoundMethodPackBase *pack) override
>   	{
> -		invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
> +		auto *argsPack = static_cast<PackType *>(pack);
> +
> +		std::apply([&](Args &&...args) {
> +			if constexpr (!std::is_void_v<R>)
> +				argsPack->ret_ = invoke(std::forward<decltype(args)>(args)...);
> +			else
> +				invoke(std::forward<decltype(args)>(args)...);
> +		}, std::move(argsPack->args_));
>   	}
>   
>   	virtual R activate(Args... args, bool deleteMethod = false) = 0;
> 
> base-commit: 500c2320619a47bd01d1ffe69ca4cc6eae6e00e8
Kieran Bingham July 9, 2026, 9:15 p.m. UTC | #2 
Quoting Laurent Pinchart (2026-05-11 12:11:21)
> Now that libcamera uses C++17, we can use std::apply to replace the
> custom implementation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Drop private invokePack() overload
> - Use lambda function and std::move() to avoid copy

This commit has upset the abi-compatibility checker a little - but it's
mostly looking like it has noticed API source changes have occured, and I
don't think there's anything here that doesn't still compile - and
otherwise seems to be abi-compatible.

I'll note it and highlight in the release notes, but please let me know
if there's anything that stands out in this patch would could be ABI
problematic.

Simply run ./utils/abi-compat.sh locally to get the report.

--
Kieran


> ---
>  include/libcamera/base/bound_method.h | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> index 9b42a8a1e8ca..f11159e8a0a4 100644
> --- a/include/libcamera/base/bound_method.h
> +++ b/include/libcamera/base/bound_method.h
> @@ -90,25 +90,19 @@ class BoundMethodArgs : public BoundMethodBase
>  public:
>         using PackType = BoundMethodPack<R, Args...>;
>  
> -private:
> -       template<std::size_t... I>
> -       void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> -       {
> -               [[maybe_unused]] auto *args = static_cast<PackType *>(pack);
> -
> -               if constexpr (!std::is_void_v<R>)
> -                       args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...);
> -               else
> -                       invoke(std::forward<Args>(std::get<I>(args->args_))...);
> -       }
> -
> -public:
>         BoundMethodArgs(void *obj, Object *object, ConnectionType type)
>                 : BoundMethodBase(obj, object, type) {}
>  
>         void invokePack(BoundMethodPackBase *pack) override
>         {
> -               invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
> +               auto *argsPack = static_cast<PackType *>(pack);
> +
> +               std::apply([&](Args &&...args) {
> +                       if constexpr (!std::is_void_v<R>)
> +                               argsPack->ret_ = invoke(std::forward<decltype(args)>(args)...);
> +                       else
> +                               invoke(std::forward<decltype(args)>(args)...);
> +               }, std::move(argsPack->args_));
>         }
>  
>         virtual R activate(Args... args, bool deleteMethod = false) = 0;
> 
> base-commit: 500c2320619a47bd01d1ffe69ca4cc6eae6e00e8
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart July 9, 2026, 10:33 p.m. UTC | #3 
On Thu, Jul 09, 2026 at 10:15:09PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2026-05-11 12:11:21)
> > Now that libcamera uses C++17, we can use std::apply to replace the
> > custom implementation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Drop private invokePack() overload
> > - Use lambda function and std::move() to avoid copy
> 
> This commit has upset the abi-compatibility checker a little - but it's
> mostly looking like it has noticed API source changes have occured, and I
> don't think there's anything here that doesn't still compile - and
> otherwise seems to be abi-compatible.
> 
> I'll note it and highlight in the release notes, but please let me know
> if there's anything that stands out in this patch would could be ABI
> problematic.
> 
> Simply run ./utils/abi-compat.sh locally to get the report.

I gave it a try, and it doesn't report anything related to
bound_method.h. I however get a report about SensorConfiguration that
seems quite bogus, so there may be an issue with my local setup.

> > ---
> >  include/libcamera/base/bound_method.h | 22 ++++++++--------------
> >  1 file changed, 8 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> > index 9b42a8a1e8ca..f11159e8a0a4 100644
> > --- a/include/libcamera/base/bound_method.h
> > +++ b/include/libcamera/base/bound_method.h
> > @@ -90,25 +90,19 @@ class BoundMethodArgs : public BoundMethodBase
> >  public:
> >         using PackType = BoundMethodPack<R, Args...>;
> >  
> > -private:
> > -       template<std::size_t... I>
> > -       void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> > -       {
> > -               [[maybe_unused]] auto *args = static_cast<PackType *>(pack);
> > -
> > -               if constexpr (!std::is_void_v<R>)
> > -                       args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...);
> > -               else
> > -                       invoke(std::forward<Args>(std::get<I>(args->args_))...);
> > -       }
> > -
> > -public:
> >         BoundMethodArgs(void *obj, Object *object, ConnectionType type)
> >                 : BoundMethodBase(obj, object, type) {}
> >  
> >         void invokePack(BoundMethodPackBase *pack) override
> >         {
> > -               invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
> > +               auto *argsPack = static_cast<PackType *>(pack);
> > +
> > +               std::apply([&](Args &&...args) {
> > +                       if constexpr (!std::is_void_v<R>)
> > +                               argsPack->ret_ = invoke(std::forward<decltype(args)>(args)...);
> > +                       else
> > +                               invoke(std::forward<decltype(args)>(args)...);
> > +               }, std::move(argsPack->args_));
> >         }
> >  
> >         virtual R activate(Args... args, bool deleteMethod = false) = 0;
> > 
> > base-commit: 500c2320619a47bd01d1ffe69ca4cc6eae6e00e8
Kieran Bingham July 10, 2026, 6:56 a.m. UTC | #4 
Quoting Laurent Pinchart (2026-07-09 23:33:47)
> On Thu, Jul 09, 2026 at 10:15:09PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2026-05-11 12:11:21)
> > > Now that libcamera uses C++17, we can use std::apply to replace the
> > > custom implementation.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Drop private invokePack() overload
> > > - Use lambda function and std::move() to avoid copy
> > 
> > This commit has upset the abi-compatibility checker a little - but it's
> > mostly looking like it has noticed API source changes have occured, and I
> > don't think there's anything here that doesn't still compile - and
> > otherwise seems to be abi-compatible.
> > 
> > I'll note it and highlight in the release notes, but please let me know
> > if there's anything that stands out in this patch would could be ABI
> > problematic.
> > 
> > Simply run ./utils/abi-compat.sh locally to get the report.
> 
> I gave it a try, and it doesn't report anything related to
> bound_method.h. I however get a report about SensorConfiguration that
> seems quite bogus, so there may be an issue with my local setup.

Yes, that's what happens. But it only happens after this commit.

I think we can probably call it a false positive? I don't think changing
things in bound_method can cause any real change in the
SensorConfiguration structures.

--
Kieran

> 
> > > ---
> > >  include/libcamera/base/bound_method.h | 22 ++++++++--------------
> > >  1 file changed, 8 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
> > > index 9b42a8a1e8ca..f11159e8a0a4 100644
> > > --- a/include/libcamera/base/bound_method.h
> > > +++ b/include/libcamera/base/bound_method.h
> > > @@ -90,25 +90,19 @@ class BoundMethodArgs : public BoundMethodBase
> > >  public:
> > >         using PackType = BoundMethodPack<R, Args...>;
> > >  
> > > -private:
> > > -       template<std::size_t... I>
> > > -       void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
> > > -       {
> > > -               [[maybe_unused]] auto *args = static_cast<PackType *>(pack);
> > > -
> > > -               if constexpr (!std::is_void_v<R>)
> > > -                       args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...);
> > > -               else
> > > -                       invoke(std::forward<Args>(std::get<I>(args->args_))...);
> > > -       }
> > > -
> > > -public:
> > >         BoundMethodArgs(void *obj, Object *object, ConnectionType type)
> > >                 : BoundMethodBase(obj, object, type) {}
> > >  
> > >         void invokePack(BoundMethodPackBase *pack) override
> > >         {
> > > -               invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
> > > +               auto *argsPack = static_cast<PackType *>(pack);
> > > +
> > > +               std::apply([&](Args &&...args) {
> > > +                       if constexpr (!std::is_void_v<R>)
> > > +                               argsPack->ret_ = invoke(std::forward<decltype(args)>(args)...);
> > > +                       else
> > > +                               invoke(std::forward<decltype(args)>(args)...);
> > > +               }, std::move(argsPack->args_));
> > >         }
> > >  
> > >         virtual R activate(Args... args, bool deleteMethod = false) = 0;
> > > 
> > > base-commit: 500c2320619a47bd01d1ffe69ca4cc6eae6e00e8
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h
index 9b42a8a1e8ca..f11159e8a0a4 100644
--- a/include/libcamera/base/bound_method.h
+++ b/include/libcamera/base/bound_method.h
@@ -90,25 +90,19 @@  class BoundMethodArgs : public BoundMethodBase
 public:
 	using PackType = BoundMethodPack<R, Args...>;
 
-private:
-	template<std::size_t... I>
-	void invokePack(BoundMethodPackBase *pack, std::index_sequence<I...>)
-	{
-		[[maybe_unused]] auto *args = static_cast<PackType *>(pack);
-
-		if constexpr (!std::is_void_v<R>)
-			args->ret_ = invoke(std::forward<Args>(std::get<I>(args->args_))...);
-		else
-			invoke(std::forward<Args>(std::get<I>(args->args_))...);
-	}
-
-public:
 	BoundMethodArgs(void *obj, Object *object, ConnectionType type)
 		: BoundMethodBase(obj, object, type) {}
 
 	void invokePack(BoundMethodPackBase *pack) override
 	{
-		invokePack(pack, std::make_index_sequence<sizeof...(Args)>{});
+		auto *argsPack = static_cast<PackType *>(pack);
+
+		std::apply([&](Args &&...args) {
+			if constexpr (!std::is_void_v<R>)
+				argsPack->ret_ = invoke(std::forward<decltype(args)>(args)...);
+			else
+				invoke(std::forward<decltype(args)>(args)...);
+		}, std::move(argsPack->args_));
 	}
 
 	virtual R activate(Args... args, bool deleteMethod = false) = 0;