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

[Umang Jain]

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.
>
>
>> [...]
>
> Regards,
> Barnabás Pőcze