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

[Umang Jain]

Hi again

On 2/15/23 1:23 AM, Umang Jain via libcamera-devel wrote:
> 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

Ah sorry, it's the `for` block not `if`

The {....} makes sense here, sorry for the noise!
>> 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.
>