| Message ID | 20250924124713.3361707-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. Quoting Barnabás Pőcze (2025-09-24 14:47:06) > Having extra qualifiers, especially `const`, simply inhibits optimization > opportunities as it prevents e.g. moving values out of pack object. So > strip the qualifiers. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> I fully trust you on this one :-) Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > --- > 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 91fe8b8cb..cb642e0a9 100644 > --- a/include/libcamera/base/bound_method.h > +++ b/include/libcamera/base/bound_method.h > @@ -39,7 +39,7 @@ public: > { > } > > - std::tuple<typename std::remove_reference_t<Args>...> args_; > + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > R ret_; > }; > > @@ -53,7 +53,7 @@ public: > { > } > > - std::tuple<typename std::remove_reference_t<Args>...> args_; > + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > }; > > class BoundMethodBase > -- > 2.51.0 >
Quoting Stefan Klug (2025-10-06 13:50:16) > Hi Barnabás, > > Thank you for the patch. > > Quoting Barnabás Pőcze (2025-09-24 14:47:06) > > Having extra qualifiers, especially `const`, simply inhibits optimization > > opportunities as it prevents e.g. moving values out of pack object. So > > strip the qualifiers. > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > I fully trust you on this one :-) > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Cheers, > Stefan > > > --- > > 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 91fe8b8cb..cb642e0a9 100644 > > --- a/include/libcamera/base/bound_method.h > > +++ b/include/libcamera/base/bound_method.h > > @@ -39,7 +39,7 @@ public: > > { > > } > > > > - std::tuple<typename std::remove_reference_t<Args>...> args_; > > + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; While trying to understand the implications of this I also came across std::decay - https://en.cppreference.com/w/cpp/types/decay.html Is that equivalent to what we're trying to achieve here? And there's also https://en.cppreference.com/w/cpp/types/remove_cvref.html - but that's C++20 only... -- Kieran > > R ret_; > > }; > > > > @@ -53,7 +53,7 @@ public: > > { > > } > > > > - std::tuple<typename std::remove_reference_t<Args>...> args_; > > + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > > }; > > > > class BoundMethodBase > > -- > > 2.51.0 > >
2025. 10. 07. 10:38 keltezéssel, Kieran Bingham írta: > Quoting Stefan Klug (2025-10-06 13:50:16) >> Hi Barnabás, >> >> Thank you for the patch. >> >> Quoting Barnabás Pőcze (2025-09-24 14:47:06) >>> Having extra qualifiers, especially `const`, simply inhibits optimization >>> opportunities as it prevents e.g. moving values out of pack object. So >>> strip the qualifiers. >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> >> I fully trust you on this one :-) >> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> >> >> Cheers, >> Stefan >> >>> --- >>> 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 91fe8b8cb..cb642e0a9 100644 >>> --- a/include/libcamera/base/bound_method.h >>> +++ b/include/libcamera/base/bound_method.h >>> @@ -39,7 +39,7 @@ public: >>> { >>> } >>> >>> - std::tuple<typename std::remove_reference_t<Args>...> args_; >>> + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > > While trying to understand the implications of this I also came across > std::decay - https://en.cppreference.com/w/cpp/types/decay.html > > Is that equivalent to what we're trying to achieve here? That's a good point. I think that is essentially the intention. My only concern is arrays because those decay into a simple pointer, which kind of goes against the "copy" behaviour. And crucially, this part is only used for function calls, so `Args` is argument types. Those cannot be arrays or functions. So in that case `std::decay` is equivalent to `std::remove_cvref`. Regards, Barnabás Pőcze > > And there's also > https://en.cppreference.com/w/cpp/types/remove_cvref.html - but that's > C++20 only... > > -- > Kieran > >>> R ret_; >>> }; >>> >>> @@ -53,7 +53,7 @@ public: >>> { >>> } >>> >>> - std::tuple<typename std::remove_reference_t<Args>...> args_; >>> + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; >>> }; >>> >>> class BoundMethodBase >>> -- >>> 2.51.0 >>>
Quoting Barnabás Pőcze (2025-10-07 11:50:44) > 2025. 10. 07. 10:38 keltezéssel, Kieran Bingham írta: > > Quoting Stefan Klug (2025-10-06 13:50:16) > >> Hi Barnabás, > >> > >> Thank you for the patch. > >> > >> Quoting Barnabás Pőcze (2025-09-24 14:47:06) > >>> Having extra qualifiers, especially `const`, simply inhibits optimization > >>> opportunities as it prevents e.g. moving values out of pack object. So > >>> strip the qualifiers. > >>> > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> > >> I fully trust you on this one :-) > >> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > >> > >> Cheers, > >> Stefan > >> > >>> --- > >>> 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 91fe8b8cb..cb642e0a9 100644 > >>> --- a/include/libcamera/base/bound_method.h > >>> +++ b/include/libcamera/base/bound_method.h > >>> @@ -39,7 +39,7 @@ public: > >>> { > >>> } > >>> > >>> - std::tuple<typename std::remove_reference_t<Args>...> args_; > >>> + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > > > > While trying to understand the implications of this I also came across > > std::decay - https://en.cppreference.com/w/cpp/types/decay.html > > > > Is that equivalent to what we're trying to achieve here? > > That's a good point. I think that is essentially the intention. My only > concern is arrays because those decay into a simple pointer, which kind > of goes against the "copy" behaviour. > > And crucially, this part is only used for function calls, so `Args` is > argument types. Those cannot be arrays or functions. So in that case > `std::decay` is equivalent to `std::remove_cvref`. Well, I think I understand the intent now so which ever way you choose on this for your best/preferred fit: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Regards, > Barnabás Pőcze > > > > > And there's also > > https://en.cppreference.com/w/cpp/types/remove_cvref.html - but that's > > C++20 only... > > > > -- > > Kieran > > > >>> R ret_; > >>> }; > >>> > >>> @@ -53,7 +53,7 @@ public: > >>> { > >>> } > >>> > >>> - std::tuple<typename std::remove_reference_t<Args>...> args_; > >>> + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; > >>> }; > >>> > >>> class BoundMethodBase > >>> -- > >>> 2.51.0 > >>> >
diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h index 91fe8b8cb..cb642e0a9 100644 --- a/include/libcamera/base/bound_method.h +++ b/include/libcamera/base/bound_method.h @@ -39,7 +39,7 @@ public: { } - std::tuple<typename std::remove_reference_t<Args>...> args_; + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; R ret_; }; @@ -53,7 +53,7 @@ public: { } - std::tuple<typename std::remove_reference_t<Args>...> args_; + std::tuple<std::remove_cv_t<std::remove_reference_t<Args>>...> args_; }; class BoundMethodBase
Having extra qualifiers, especially `const`, simply inhibits optimization opportunities as it prevents e.g. moving values out of pack object. So strip the qualifiers. 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(-)