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

[Umang Jain]

Hi Laurent,

On 2/15/23 1:21 AM, Laurent Pinchart wrote:
> 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

comment is before(outside?) the `if` block so I don't feel the need for 
{...}

If the comment was inside the `if`, {..} makes sense
> 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.