| Message ID | 20240520040249.209200-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2024-05-20 05:02:50) > This way the construction of the default value of type `T` > can be delayed until it is really needed, which is useful, > for example when `T == std::string`, as the default value > string would always be constructed otherwise. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> This looks and sounds good to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/yaml_parser.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > index b6979d73..3ac27e06 100644 > --- a/include/libcamera/internal/yaml_parser.h > +++ b/include/libcamera/internal/yaml_parser.h > @@ -179,10 +179,10 @@ public: > #endif > std::optional<T> get() const; > > - template<typename T> > - T get(const T &defaultValue) const > + template<typename T, typename U> > + T get(U &&defaultValue) const > { > - return get<T>().value_or(defaultValue); > + return get<T>().value_or(std::forward<U>(defaultValue)); > } > > #ifndef __DOXYGEN__ > -- > 2.45.1 > >
Hi Barnabás, Thank you for the patch. On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > This way the construction of the default value of type `T` > can be delayed until it is really needed, which is useful, > for example when `T == std::string`, as the default value > string would always be constructed otherwise. That's interesting. Is that all it takes to defer the construction of the default value ? I implemented a different solution a while ago, which was merged in 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify std::optional::value_or() usage") but had to be reverted due to compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to Debian 10 still being supported, although we may drop that once it reaches EOL at end of June this year). > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > include/libcamera/internal/yaml_parser.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > index b6979d73..3ac27e06 100644 > --- a/include/libcamera/internal/yaml_parser.h > +++ b/include/libcamera/internal/yaml_parser.h > @@ -179,10 +179,10 @@ public: > #endif > std::optional<T> get() const; > > - template<typename T> > - T get(const T &defaultValue) const > + template<typename T, typename U> > + T get(U &&defaultValue) const > { > - return get<T>().value_or(defaultValue); > + return get<T>().value_or(std::forward<U>(defaultValue)); > } > > #ifndef __DOXYGEN__
2024. május 21., kedd 16:18 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > > This way the construction of the default value of type `T` > > can be delayed until it is really needed, which is useful, > > for example when `T == std::string`, as the default value > > string would always be constructed otherwise. > > That's interesting. Is that all it takes to defer the construction of > the default value ? The motivating example is std::string, previously x.get<std::string>("asd"); would force the construction of a temporary string object from the string literal "asd" since the argument type is `const std::string&`. Note that if the optional was empty, it would also force the copy construction of another std::string object, which would be the one returned. See the second overload on https://en.cppreference.com/w/cpp/utility/optional/value_or : Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). Now the value with type `T` is only constructed from `default_value` if the optional is empty. This works nicely with std::string as now a string literal can be passed as argument, and an std::string object will only be constructed from it if needed. > > I implemented a different solution a while ago, which was merged in > 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify > std::optional::value_or() usage") but had to be reverted due to > compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to > Debian 10 still being supported, although we may drop that once it > reaches EOL at end of June this year). I am fairly sure it should work, although I haven't tested it on gcc 8, this is a reasonably non-intrusive change in my opinion, the argument is simply forwarded to value_or() as is. I am wondering if there are any plans to accept contributions via gitlab merge requests as in those cases the author could immediately see the result of CI and make the necessary changes. Regards, Barnabás Pőcze > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > include/libcamera/internal/yaml_parser.h | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > > index b6979d73..3ac27e06 100644 > > --- a/include/libcamera/internal/yaml_parser.h > > +++ b/include/libcamera/internal/yaml_parser.h > > @@ -179,10 +179,10 @@ public: > > #endif > > std::optional<T> get() const; > > > > - template<typename T> > > - T get(const T &defaultValue) const > > + template<typename T, typename U> > > + T get(U &&defaultValue) const > > { > > - return get<T>().value_or(defaultValue); > > + return get<T>().value_or(std::forward<U>(defaultValue)); > > } > > > > #ifndef __DOXYGEN__ > > -- > Regards, > > Laurent Pinchart >
2024. május 21., kedd 17:40 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta: > 2024. május 21., kedd 16:18 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > > > Hi Barnabás, > > > > Thank you for the patch. > > > > On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > > > This way the construction of the default value of type `T` > > > can be delayed until it is really needed, which is useful, > > > for example when `T == std::string`, as the default value > > > string would always be constructed otherwise. > > > > That's interesting. Is that all it takes to defer the construction of > > the default value ? > > The motivating example is std::string, previously > > x.get<std::string>("asd"); > > would force the construction of a temporary string object from the string literal > "asd" since the argument type is `const std::string&`. Note that if the optional > was empty, it would also force the copy construction of another std::string object, > which would be the one returned. > > See the second overload on https://en.cppreference.com/w/cpp/utility/optional/value_or : > > Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). > > Now the value with type `T` is only constructed from `default_value` if the optional is empty. > > This works nicely with std::string as now a string literal can be passed as argument, > and an std::string object will only be constructed from it if needed. > > > > > > I implemented a different solution a while ago, which was merged in > > 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify > > std::optional::value_or() usage") but had to be reverted due to > > compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to > > Debian 10 still being supported, although we may drop that once it > > reaches EOL at end of June this year). > > I am fairly sure it should work, although I haven't tested it on gcc 8, > this is a reasonably non-intrusive change in my opinion, the argument is simply > forwarded to value_or() as is. > > I am wondering if there are any plans to accept contributions via gitlab merge > requests as in those cases the author could immediately see the result of > CI and make the necessary changes. > [...] Anything else I should do here? Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2024-06-11 22:31:16) > 2024. május 21., kedd 17:40 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta: > > > 2024. május 21., kedd 16:18 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > > > > > Hi Barnabás, > > > > > > Thank you for the patch. > > > > > > On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > > > > This way the construction of the default value of type `T` > > > > can be delayed until it is really needed, which is useful, > > > > for example when `T == std::string`, as the default value > > > > string would always be constructed otherwise. > > > > > > That's interesting. Is that all it takes to defer the construction of > > > the default value ? > > > > The motivating example is std::string, previously > > > > x.get<std::string>("asd"); > > > > would force the construction of a temporary string object from the string literal > > "asd" since the argument type is `const std::string&`. Note that if the optional > > was empty, it would also force the copy construction of another std::string object, > > which would be the one returned. > > > > See the second overload on https://en.cppreference.com/w/cpp/utility/optional/value_or : > > > > Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). > > > > Now the value with type `T` is only constructed from `default_value` if the optional is empty. > > > > This works nicely with std::string as now a string literal can be passed as argument, > > and an std::string object will only be constructed from it if needed. > > > > > > > > > > I implemented a different solution a while ago, which was merged in > > > 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify > > > std::optional::value_or() usage") but had to be reverted due to > > > compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to > > > Debian 10 still being supported, although we may drop that once it > > > reaches EOL at end of June this year). > > > > I am fairly sure it should work, although I haven't tested it on gcc 8, > > this is a reasonably non-intrusive change in my opinion, the argument is simply > > forwarded to value_or() as is. > > > > I am wondering if there are any plans to accept contributions via gitlab merge > > requests as in those cases the author could immediately see the result of > > CI and make the necessary changes. I'm working on getting the CI kicked from patches sent to the list 'automatically'* *Automatically - with approval from someone with commit access to prevent potential abuses. But meanwhile: ./send-for-testing.sh 4319 - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1199972 : pending : patchwork/4319 Does report some documentation issues: - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59823569#L899 I believe we can also give access to regular contributors to either be able to enable the CI on their own forks or have push rights to a CI build on gitlab. An option like that could be arranged for you perhaps? -- Kieran > > [...] > > Anything else I should do here? > > > Regards, > Barnabás Pőcze
2024. június 13., csütörtök 0:24 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze (2024-06-11 22:31:16) > > 2024. május 21., kedd 17:40 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta: > > > > > 2024. május 21., kedd 16:18 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > > > > > > > Hi Barnabás, > > > > > > > > Thank you for the patch. > > > > > > > > On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > > > > > This way the construction of the default value of type `T` > > > > > can be delayed until it is really needed, which is useful, > > > > > for example when `T == std::string`, as the default value > > > > > string would always be constructed otherwise. > > > > > > > > That's interesting. Is that all it takes to defer the construction of > > > > the default value ? > > > > > > The motivating example is std::string, previously > > > > > > x.get<std::string>("asd"); > > > > > > would force the construction of a temporary string object from the string literal > > > "asd" since the argument type is `const std::string&`. Note that if the optional > > > was empty, it would also force the copy construction of another std::string object, > > > which would be the one returned. > > > > > > See the second overload on https://en.cppreference.com/w/cpp/utility/optional/value_or : > > > > > > Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). > > > > > > Now the value with type `T` is only constructed from `default_value` if the optional is empty. > > > > > > This works nicely with std::string as now a string literal can be passed as argument, > > > and an std::string object will only be constructed from it if needed. > > > > > > > > > > > > > > I implemented a different solution a while ago, which was merged in > > > > 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify > > > > std::optional::value_or() usage") but had to be reverted due to > > > > compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to > > > > Debian 10 still being supported, although we may drop that once it > > > > reaches EOL at end of June this year). > > > > > > I am fairly sure it should work, although I haven't tested it on gcc 8, > > > this is a reasonably non-intrusive change in my opinion, the argument is simply > > > forwarded to value_or() as is. > > > > > > I am wondering if there are any plans to accept contributions via gitlab merge > > > requests as in those cases the author could immediately see the result of > > > CI and make the necessary changes. > > I'm working on getting the CI kicked from patches sent to the list > 'automatically'* > > *Automatically - with approval from someone with commit access to > prevent potential abuses. > > But meanwhile: > > ./send-for-testing.sh 4319 > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1199972 : pending : patchwork/4319 > > Does report some documentation issues: > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59823569#L899 Oops, I forgot to add the documentation changes to the commit... I will send a new version. > > I believe we can also give access to regular contributors to either be > able to enable the CI on their own forks or have push rights to a CI > build on gitlab. An option like that could be arranged for you perhaps? Well, my preference is being able to send changes via gitlab. I understand this is not the project's preference, but so far nothing has shaken me in my belief that a merge request with automatic CI is most convenient for contributors. Regards, Barnabás Pőcze > [...]
Hi Barnabás, On Tue, May 21, 2024 at 03:40:13PM +0000, Barnabás Pőcze wrote: > 2024. május 21., kedd 16:18 keltezéssel, Laurent Pinchart írta: > > On Mon, May 20, 2024 at 04:02:50AM +0000, Barnabás Pőcze wrote: > > > This way the construction of the default value of type `T` > > > can be delayed until it is really needed, which is useful, > > > for example when `T == std::string`, as the default value > > > string would always be constructed otherwise. > > > > That's interesting. Is that all it takes to defer the construction of > > the default value ? > > The motivating example is std::string, previously > > x.get<std::string>("asd"); > > would force the construction of a temporary string object from the string literal > "asd" since the argument type is `const std::string&`. Note that if the optional > was empty, it would also force the copy construction of another std::string object, > which would be the one returned. > > See the second overload on https://en.cppreference.com/w/cpp/utility/optional/value_or : > > Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). > > Now the value with type `T` is only constructed from `default_value` if the optional is empty. > > This works nicely with std::string as now a string literal can be passed as argument, > and an std::string object will only be constructed from it if needed. Aahhhh thanks for the explanation. So this won't prevent the std::string construction for x.get<std::string>(std::string{"abc"}) but it will for x.get<std::string>("abc") (assuming x is not empty of course). That's a useful change I think. > > I implemented a different solution a while ago, which was merged in > > 48c106429a19 ("libcamera: base: utils: Provide defopt to simplify > > std::optional::value_or() usage") but had to be reverted due to > > compilation issues with gcc 8.0.0 to 8.3.0 (which we care about due to > > Debian 10 still being supported, although we may drop that once it > > reaches EOL at end of June this year). > > I am fairly sure it should work, although I haven't tested it on gcc 8, > this is a reasonably non-intrusive change in my opinion, the argument is simply > forwarded to value_or() as is. I think it's valid C++ code, but gcc 8.0.0 to 8.3.0 unfortunately didn't agree :-( I'm tempted to revive the code at some point this year, if we decide to drop support for Debian 10. > I am wondering if there are any plans to accept contributions via gitlab merge > requests as in those cases the author could immediately see the result of > CI and make the necessary changes. We are considering different options to improve the contribution model, but for the time being we want to keep the mailing list workflow for reviews. One idea that was floated around was a merge request to e-mail gateway (one direction only, reviews would then move to e-mail, they wouldn't be forwarded to gitlab). Due to the amount of CI abuse on gitlab.fdo, I don't think we'll be able to automatically trigger pipeline runs when a merge request is submitted by a non-member though. Manual pre-review and approval of the pipeline run will likely be needed. If you're a member of the camera CI group (or would like to become one) then you could already have access to CI through your own clone. Another thing we are considering is automating the creation of CI pipelines from patch series posted to the list. It will still require manual approval from maintainers due to the issue listed above, but it would still be a good step forward. There's a high chance this will get implemented in the not too distant future. > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > include/libcamera/internal/yaml_parser.h | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h > > > index b6979d73..3ac27e06 100644 > > > --- a/include/libcamera/internal/yaml_parser.h > > > +++ b/include/libcamera/internal/yaml_parser.h > > > @@ -179,10 +179,10 @@ public: > > > #endif > > > std::optional<T> get() const; > > > > > > - template<typename T> > > > - T get(const T &defaultValue) const > > > + template<typename T, typename U> > > > + T get(U &&defaultValue) const > > > { > > > - return get<T>().value_or(defaultValue); > > > + return get<T>().value_or(std::forward<U>(defaultValue)); > > > } > > > > > > #ifndef __DOXYGEN__
diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h index b6979d73..3ac27e06 100644 --- a/include/libcamera/internal/yaml_parser.h +++ b/include/libcamera/internal/yaml_parser.h @@ -179,10 +179,10 @@ public: #endif std::optional<T> get() const; - template<typename T> - T get(const T &defaultValue) const + template<typename T, typename U> + T get(U &&defaultValue) const { - return get<T>().value_or(defaultValue); + return get<T>().value_or(std::forward<U>(defaultValue)); } #ifndef __DOXYGEN__
This way the construction of the default value of type `T` can be delayed until it is really needed, which is useful, for example when `T == std::string`, as the default value string would always be constructed otherwise. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- include/libcamera/internal/yaml_parser.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)