[libcamera-devel,v2] libcamera: yaml_parser: Return nullopt on error
diff mbox series

Message ID 20220803112937.30566-1-jacopo@jmondi.org
State Accepted
Commit 2e77ccbb93a14a0c976bb3ca9d5dbbdc32a12847
Headers show
Series
  • [libcamera-devel,v2] libcamera: yaml_parser: Return nullopt on error
Related show

Commit Message

Jacopo Mondi Aug. 3, 2022, 11:29 a.m. UTC 
The YamlParser::getList<>() function returns an std::optional<> to allow
callers to identify cases where parsing the .yaml file failed from cases
where the parsed list is just empty.

The current implementation returns an empty vector in case of parsing
errors, which evaluates to std::nullopt but it less explicit.

Change this by returning std::nullopt in case of errors to make the
intended behaviour more explicit.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/yaml_parser.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Aug. 3, 2022, 11:37 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Wed, Aug 03, 2022 at 01:29:37PM +0200, Jacopo Mondi via libcamera-devel wrote:
> The YamlParser::getList<>() function returns an std::optional<> to allow
> callers to identify cases where parsing the .yaml file failed from cases
> where the parsed list is just empty.
> 
> The current implementation returns an empty vector in case of parsing
> errors, which evaluates to std::nullopt but it less explicit.

{} is not an empty vector, as it's not a vector :-)


The current implementation returns a default-constructed std::optional
in case of errors with

	return {};

The returned value is thus equal to std::nullopt, but the code can be
easily misinterpreted as returning an empty vector by a reader.



Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Change this by returning std::nullopt in case of errors to make the
> intended behaviour more explicit.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 84cb57d6de83..c96e99e1317c 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -319,7 +319,7 @@ template<typename T,
>  std::optional<std::vector<T>> YamlObject::getList() const
>  {
>  	if (type_ != Type::List)
> -		return {};
> +		return std::nullopt;
>  
>  	std::vector<T> values;
>  	values.reserve(list_.size());
> @@ -327,7 +327,7 @@ std::optional<std::vector<T>> YamlObject::getList() const
>  	for (const YamlObject &entry : asList()) {
>  		const auto value = entry.get<T>();
>  		if (!value)
> -			return {};
> +			return std::nullopt;
>  		values.emplace_back(*value);
>  	}
>

Patch
diff mbox series 

diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
index 84cb57d6de83..c96e99e1317c 100644
--- a/src/libcamera/yaml_parser.cpp
+++ b/src/libcamera/yaml_parser.cpp
@@ -319,7 +319,7 @@  template<typename T,
 std::optional<std::vector<T>> YamlObject::getList() const
 {
 	if (type_ != Type::List)
-		return {};
+		return std::nullopt;
 
 	std::vector<T> values;
 	values.reserve(list_.size());
@@ -327,7 +327,7 @@  std::optional<std::vector<T>> YamlObject::getList() const
 	for (const YamlObject &entry : asList()) {
 		const auto value = entry.get<T>();
 		if (!value)
-			return {};
+			return std::nullopt;
 		values.emplace_back(*value);
 	}