[libcamera-devel] [PATCH] libcamera: yaml_parser: Return nullopt on error

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 03, 2022 at 01:03:24PM +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 however returns an empty list in case of
> errors, making it impossible for the caller to detect parsing failures.

That's actually not true:

--------
#include <iostream>
#include <optional>
#include <vector>

static std::optional<std::vector<int>> foo(bool null)
{
	if (null)
		return std::nullopt;
	else
		return {};
}

int main()
{
	std::optional<std::vector<int>> opt;

	opt = foo(true);
	std::cout << "- opt " << (opt == std::nullopt ? "==" : "!=") << " std::nullopt" << std::endl;

	opt = foo(false);
	std::cout << "- opt " << (opt == std::nullopt ? "==" : "!=") << " std::nullopt" << std::endl;

	return 0;
}
--------

prints

--------
- opt == std::nullopt
- opt == std::nullopt
--------

{} means a default-constructed object, whose type is inferred from the
return type of the function here, so std::optional<std::vector<int>>()
in this case, which is equal to std::nullopt.

However, I agree that std::nullopt is more explicit, so I like the
change, but the commit message should be reworded.

> Fix this by returning std::nullopt in case of errors.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  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);
>  	}
>  

-- 
Regards,

Laurent Pinchart