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

[Laurent Pinchart]

On Tue, Feb 14, 2023 at 10:33:58PM +0530, Umang Jain via libcamera-devel wrote:
> On 2/14/23 7:10 PM, Barnabás Pőcze wrote:
> > 2023. február 14., kedd 8:03 keltezéssel, Umang Jain í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.

It's a matter of style. I tend to keep the curly braces when the content
has a comment in addition to the statement, but drop them when the
statement spans multiple lines. I'm not sure there's any reason for that
beside being used to it :-) It's thus quite personal, and I'm OK with
either.

> I would just drop it.

-- 
Regards,

Laurent Pinchart