[PATCH v1] libcamera: yaml_parser: Make default value templated in `get()`

[Laurent Pinchart]

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 at 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