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

[Umang Jain]

Hi Barnabás,

Thank you for the patch.

In the subject line:
     s/return/Return

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

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
>   	return roles;
> @@ -108,27 +101,21 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
>   	return 0;
>   }
>
> -bool StreamKeyValueParser::parseRole(StreamRole *role,
> -				     const KeyValueParser::Options &options)
> +std::optional<libcamera::StreamRole> StreamKeyValueParser::parseRole(const KeyValueParser::Options &options)
>   {
>   	if (!options.isSet("role"))
> -		return false;
> +		return {};
>
>   	std::string name = options["role"].toString();
>
> -	if (name == "viewfinder") {
> -		*role = StreamRole::Viewfinder;
> -		return true;
> -	} else if (name == "video") {
> -		*role = StreamRole::VideoRecording;
> -		return true;
> -	} else if (name == "still") {
> -		*role = StreamRole::StillCapture;
> -		return true;
> -	} else if (name == "raw") {
> -		*role = StreamRole::Raw;
> -		return true;
> -	}
> +	if (name == "viewfinder")
> +		return StreamRole::Viewfinder;
> +	else if (name == "video")
> +		return StreamRole::VideoRecording;
> +	else if (name == "still")
> +		return StreamRole::StillCapture;
> +	else if (name == "raw")
> +		return StreamRole::Raw;
>
> -	return false;
> +	return {};
>   }
> diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
> index 35e4e7c0..fe298c84 100644
> --- a/src/apps/common/stream_options.h
> +++ b/src/apps/common/stream_options.h
> @@ -7,6 +7,8 @@
>
>   #pragma once
>
> +#include <optional>
> +
>   #include <libcamera/camera.h>
>
>   #include "options.h"
> @@ -23,6 +25,5 @@ public:
>   				       const OptionValue &values);
>
>   private:
> -	static bool parseRole(libcamera::StreamRole *role,
> -			      const KeyValueParser::Options &options);
> +	static std::optional<libcamera::StreamRole> parseRole(const KeyValueParser::Options &options);
>   };
> --
> 2.39.1
>
>