[libcamera-devel] [PATCH v1] apps: return std::optional from StreamKeyValueParser::parseRole()

[Jacopo Mondi]

Hello

On Tue, Feb 14, 2023 at 10:33:58PM +0530, Umang Jain via libcamera-devel wrote:
> Hi,
>
> On 2/14/23 7:10 PM, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2023. február 14., kedd 8:03 keltezéssel, Umang Jain <umang.jain at ideasonboard.com> írta:
> >
> > > Hi Barnabás,
> > >
> > > Thank you for the patch.
> > >
> > > In the subject line:
> > >       s/return/Return
> > ACK
> >
> >
> > > On 2/13/23 10:13 PM, Barnabás Pőcze wrote:
> > > > Instead of having bool return type and an out parameter,
> > > > just use std::optional.
> > > >
> > > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > > ---
> > > >    src/apps/common/stream_options.cpp | 39 ++++++++++--------------------
> > > >    src/apps/common/stream_options.h   |  5 ++--
> > > >    2 files changed, 16 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> > > > index 3a5625f5..d3785999 100644
> > > > --- a/src/apps/common/stream_options.cpp
> > > > +++ b/src/apps/common/stream_options.cpp
> > > > @@ -30,10 +30,8 @@ StreamKeyValueParser::StreamKeyValueParser()
> > > >    KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> > > >    {
> > > >    	KeyValueParser::Options options = KeyValueParser::parse(arguments);
> > > > -	StreamRole role;
> > > >
> > > > -	if (options.valid() && options.isSet("role") &&
> > > > -	    !parseRole(&role, options)) {
> > > > +	if (options.valid() && options.isSet("role") && !parseRole(options)) {
> > > >    		std::cerr << "Unknown stream role "
> > > >    			  << options["role"].toString() << std::endl;
> > > >    		options.invalidate();
> > > > @@ -52,13 +50,8 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> > > >
> > > >    	StreamRoles roles;
> > > >    	for (auto const &value : streamParameters) {
> > > > -		StreamRole role;
> > > > -
> > > >    		/* If role is invalid or not set default to viewfinder. */
> > > > -		if (!parseRole(&role, value.toKeyValues()))
> > > > -			role = StreamRole::Viewfinder;
> > > > -
> > > > -		roles.push_back(role);
> > > > +		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));
> > > >    	}
> > > The curly braces {...} can be dropped as well
> > I kept them because of the comment in the previous line.
> > To me it looks better this way. Should I drop them?
>
> I don't think the comment has an impact on including or not including the
> {...} braces.
>
> I would just drop it.

Or we can move the comment up. Also the comment seems a bit weird, or
am I parsing it wrong ?

  	/* If role is invalid or not set default to viewfinder. */

Can we turn it to:

  	/* If a role is invalid default it to viewfinder. */
    	for (auto const &value : streamParameters)
		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));

Anyway, no need to resend, we can fix when applying it with your ack
on whatever you prefer

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> >
> >
> > > [...]
> >
> > Regards,
> > Barnabás Pőcze
>