[libcamera-devel,v5,2/9] libcamera: yaml_parser: Replace ok flag to get() with std::optional

16850 diff mbox series

The YamlObject::get() function takes a default value and an optional
bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
error handling in callers.

A better API is possible by dropping the default value and ok flag and
returning an std::optional. Not only does it simplify the calls, it also
lets callers handle errors through the standard std::optional class
instead of the current ad-hoc mechanism.

Provide a get() wrapper around std::optional::value_or() to further
simplify callers that don't need any specific error handling.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Tested-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/internal/yaml_parser.h |   9 +-
 src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
 test/yaml-parser.cpp                     |  71 ++++++------
 3 files changed, 96 insertions(+), 122 deletions(-)

Quoting Laurent Pinchart via libcamera-devel (2022-07-27 23:21:42)
> The YamlObject::get() function takes a default value and an optional
> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> error handling in callers.
> 
> A better API is possible by dropping the default value and ok flag and
> returning an std::optional. Not only does it simplify the calls, it also
> lets callers handle errors through the standard std::optional class
> instead of the current ad-hoc mechanism.
> 
> Provide a get() wrapper around std::optional::value_or() to further
> simplify callers that don't need any specific error handling.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/yaml_parser.h |   9 +-
>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
>  test/yaml-parser.cpp                     |  71 ++++++------
>  3 files changed, 96 insertions(+), 122 deletions(-)
> 
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 064cf44381d7..61f2223223a7 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -9,6 +9,7 @@
>  

<snip>

> --- a/test/yaml-parser.cpp
> +++ b/test/yaml-parser.cpp
> @@ -148,7 +148,6 @@ protected:
>                 }
>  
>                 /* Test string object */
> -               bool ok;
>                 auto &strObj = (*root)["string"];
>  
>                 if (strObj.isDictionary()) {
> @@ -161,27 +160,27 @@ protected:
>                         return TestFail;
>                 }
>  
> -               if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> +               if (strObj.get<string>().value_or("") != "libcamera") {

Does this series mean we can now (safely?) do:

		  if (strObj.get<string>() != "libcamera") {
		  }

Or perhaps the previous patch means we could do:


		  if (strObj.get<string>().value_or(utils::defopt) != "libcamera") {
		  }

Though, that's not shorter here, hence using the "" instead?




>                         cerr << "String object parse as wrong content" << std::endl;
>                         return TestFail;
>                 }
>  
> -               if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +               if (strObj.get<int32_t>()) {
>                         cerr << "String object parse as integer" << std::endl;
>                         return TestFail;
>                 }
>  
> -               if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +               if (strObj.get<uint32_t>()) {
>                         cerr << "String object parse as unsigned integer" << std::endl;
>                         return TestFail;
>                 }
>  
> -               if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +               if (strObj.get<double>()) {
>                         cerr << "String object parse as double" << std::endl;
>                         return TestFail;
>                 }
>  
> -               if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +               if (strObj.get<Size>()) {
>                         cerr << "String object parse as Size" << std::endl;
>                         return TestFail;
>                 }

Hi Kieran,

On Wed, Jul 27, 2022 at 11:49:18PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-07-27 23:21:42)
> > The YamlObject::get() function takes a default value and an optional
> > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > error handling in callers.
> > 
> > A better API is possible by dropping the default value and ok flag and
> > returning an std::optional. Not only does it simplify the calls, it also
> > lets callers handle errors through the standard std::optional class
> > instead of the current ad-hoc mechanism.
> > 
> > Provide a get() wrapper around std::optional::value_or() to further
> > simplify callers that don't need any specific error handling.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/yaml_parser.h |   9 +-
> >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> >  test/yaml-parser.cpp                     |  71 ++++++------
> >  3 files changed, 96 insertions(+), 122 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 064cf44381d7..61f2223223a7 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -9,6 +9,7 @@
> 
> <snip>
> 
> > --- a/test/yaml-parser.cpp
> > +++ b/test/yaml-parser.cpp
> > @@ -148,7 +148,6 @@ protected:
> >                 }
> >  
> >                 /* Test string object */
> > -               bool ok;
> >                 auto &strObj = (*root)["string"];
> >  
> >                 if (strObj.isDictionary()) {
> > @@ -161,27 +160,27 @@ protected:
> >                         return TestFail;
> >                 }
> >  
> > -               if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > +               if (strObj.get<string>().value_or("") != "libcamera") {
> 
> Does this series mean we can now (safely?) do:
> 
> 		  if (strObj.get<string>() != "libcamera") {
> 		  }

This is safe, and the condition will be true of strObj doesn't contain a
string or if it contains a string other than "libcamera".

> 
> Or perhaps the previous patch means we could do:
> 
> 
> 		  if (strObj.get<string>().value_or(utils::defopt) != "libcamera") {
> 		  }

This is fine too.

> Though, that's not shorter here, hence using the "" instead?

You can indeed write

 		  if (strObj.get<string>().value_or("") != "libcamera") {
 		  }

at the cost of an unnecessary construction and destruction of a
std::string("") if strObj.get<string> has a value.

> >                         cerr << "String object parse as wrong content" << std::endl;
> >                         return TestFail;
> >                 }
> >  
> > -               if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +               if (strObj.get<int32_t>()) {
> >                         cerr << "String object parse as integer" << std::endl;
> >                         return TestFail;
> >                 }
> >  
> > -               if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +               if (strObj.get<uint32_t>()) {
> >                         cerr << "String object parse as unsigned integer" << std::endl;
> >                         return TestFail;
> >                 }
> >  
> > -               if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +               if (strObj.get<double>()) {
> >                         cerr << "String object parse as double" << std::endl;
> >                         return TestFail;
> >                 }
> >  
> > -               if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +               if (strObj.get<Size>()) {
> >                         cerr << "String object parse as Size" << std::endl;
> >                         return TestFail;
> >                 }

Hi Laurent

On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The YamlObject::get() function takes a default value and an optional
> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> error handling in callers.
>
> A better API is possible by dropping the default value and ok flag and
> returning an std::optional. Not only does it simplify the calls, it also
> lets callers handle errors through the standard std::optional class
> instead of the current ad-hoc mechanism.
>
> Provide a get() wrapper around std::optional::value_or() to further
> simplify callers that don't need any specific error handling.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  include/libcamera/internal/yaml_parser.h |   9 +-
>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
>  test/yaml-parser.cpp                     |  71 ++++++------
>  3 files changed, 96 insertions(+), 122 deletions(-)
>
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 064cf44381d7..61f2223223a7 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -9,6 +9,7 @@
>
>  #include <iterator>
>  #include <map>
> +#include <optional>
>  #include <string>
>  #include <vector>
>
> @@ -165,7 +166,13 @@ public:
>  #else
>  	template<typename T>
>  #endif
> -	T get(const T &defaultValue, bool *ok = nullptr) const;
> +	std::optional<T> get() const;
> +
> +	template<typename T>
> +	T get(const T &defaultValue) const
> +	{
> +		return get<T>().value_or(defaultValue);
> +	}
>
>  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
>  	ListAdapter asList() const { return ListAdapter{ list_ }; }
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 5c45e44e49c3..4299f5abd38a 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -31,12 +31,6 @@ namespace {
>  /* Empty static YamlObject as a safe result for invalid operations */
>  static const YamlObject empty;
>
> -void setOk(bool *ok, bool result)
> -{
> -	if (ok)
> -		*ok = result;
> -}
> -
>  } /* namespace */
>
>  /**
> @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
>  }
>
>  /**
> - * \fn template<typename T> YamlObject::get<T>(
> - *	const T &defaultValue, bool *ok) const
> + * \fn template<typename T> YamlObject::get<T>() const
> + * \brief Parse the YamlObject as a \a T value
> + *
> + * This function parses the value of the YamlObject as a \a T object, and
> + * returns the value. If parsing fails (usually because the YamlObject doesn't
> + * store a \a T value), std::nullopt is returned.
> + *
> + * \return The YamlObject value, or std::nullopt if parsing failed
> + */
> +
> +/**
> + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
>   * \brief Parse the YamlObject as a \a T value
>   * \param[in] defaultValue The default value when failing to parse
> - * \param[out] ok The result of whether the parse succeeded
>   *
>   * This function parses the value of the YamlObject as a \a T object, and
>   * returns the value. If parsing fails (usually because the YamlObject doesn't
> - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> + * store a \a T value), the \a defaultValue is returned.
>   *
> - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> - * need to know if parsing succeeded.
> - *
> - * \return Value as a bool type
> + * \return The YamlObject value, or \a defaultValue if parsing failed
>   */
>
>  #ifndef __DOXYGEN__
>
>  template<>
> -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> +std::optional<bool> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
> -	if (value_ == "true") {
> -		setOk(ok, true);
> +	if (value_ == "true")
>  		return true;
> -	} else if (value_ == "false") {
> -		setOk(ok, true);
> +	else if (value_ == "false")
>  		return false;
> -	}
>
> -	return defaultValue;
> +	return {};
>  }
>
>  template<>
> -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> +std::optional<int16_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int16_t>::min() ||
>  	    value > std::numeric_limits<int16_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> +std::optional<uint16_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint16_t>::min() ||
>  	    value > std::numeric_limits<uint16_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> +std::optional<int32_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int32_t>::min() ||
>  	    value > std::numeric_limits<int32_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> +std::optional<uint32_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint32_t>::min() ||
>  	    value > std::numeric_limits<uint32_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -double YamlObject::get(const double &defaultValue, bool *ok) const
> +std::optional<double> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
>  	double value = std::strtod(value_.c_str(), &end);
>
>  	if ('\0' != *end || errno == ERANGE)
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> +std::optional<std::string> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value_;
>  }
>
>  template<>
> -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> +std::optional<Size> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::List)
> -		return defaultValue;
> +		return {};
>
>  	if (list_.size() != 2)
> -		return defaultValue;
> +		return {};
>
> -	/*
> -	 * Add a local variable to validate each dimension in case
> -	 * that ok == nullptr.
> -	 */
> -	bool valid;
> -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> -	if (!valid)
> -		return defaultValue;
> +	auto width = list_[0]->get<uint32_t>();
> +	if (!width)
> +		return {};
>
> -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> -	if (!valid)
> -		return defaultValue;
> +	auto height = list_[1]->get<uint32_t>();
> +	if (!height)
> +		return {};
>
> -	setOk(ok, true);
> -	return Size(width, height);
> +	return Size(*width, *height);
>  }
>
>  #endif /* __DOXYGEN__ */
> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> index 38f848232fa6..ebb654f2ef9c 100644
> --- a/test/yaml-parser.cpp
> +++ b/test/yaml-parser.cpp
> @@ -148,7 +148,6 @@ protected:
>  		}
>
>  		/* Test string object */
> -		bool ok;
>  		auto &strObj = (*root)["string"];
>
>  		if (strObj.isDictionary()) {
> @@ -161,27 +160,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> +		if (strObj.get<string>().value_or("") != "libcamera") {
>  			cerr << "String object parse as wrong content" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (strObj.get<int32_t>()) {
>  			cerr << "String object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (strObj.get<uint32_t>()) {
>  			cerr << "String object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (strObj.get<double>()) {
>  			cerr << "String object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (strObj.get<Size>()) {
>  			cerr << "String object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -199,27 +198,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
>  			cerr << "Integer object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> +		if (int32Obj.get<string>().value_or("") != "-100") {
>  			cerr << "Integer object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
>  			cerr << "Integer object fail to parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (int32Obj.get<uint32_t>()) {
>  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (int32Obj.get<Size>()) {
>  			cerr << "Integer object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -237,27 +236,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
>  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> +		if (uint32Obj.get<string>().value_or("") != "100") {
>  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
>  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
>  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (uint32Obj.get<Size>()) {
>  			cerr << "Unsigned integer object parsed as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -275,27 +274,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> +		if (doubleObj.get<string>().value_or("") != "3.14159") {
>  			cerr << "Double object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
>  			cerr << "Double object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (doubleObj.get<int32_t>()) {
>  			cerr << "Double object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (doubleObj.get<uint32_t>()) {
>  			cerr << "Double object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (doubleObj.get<Size>()) {
>  			cerr << "Double object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -313,27 +312,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<string>("", &ok) != "" || ok) {
> +		if (sizeObj.get<string>()) {
>  			cerr << "Size object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (sizeObj.get<double>()) {
>  			cerr << "Size object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (sizeObj.get<int32_t>()) {
>  			cerr << "Size object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (sizeObj.get<uint32_t>()) {
>  			cerr << "Size object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
>  			cerr << "Size object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
> @@ -351,27 +350,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (listObj.get<string>("", &ok) != "" || ok) {
> +		if (listObj.get<string>()) {
>  			cerr << "List object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (listObj.get<double>()) {
>  			cerr << "List object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (listObj.get<int32_t>()) {
>  			cerr << "List object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (listObj.get<uint32_t>()) {
>  			cerr << "List object parse as unsigne integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (listObj.get<Size>()) {
>  			cerr << "String list object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -424,27 +423,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<string>("", &ok) != "" || ok) {
> +		if (dictObj.get<string>()) {
>  			cerr << "Dictionary object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (dictObj.get<double>()) {
>  			cerr << "Dictionary object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (dictObj.get<int32_t>()) {
>  			cerr << "Dictionary object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (dictObj.get<uint32_t>()) {
>  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (dictObj.get<Size>()) {
>  			cerr << "Dictionary object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> --
> Regards,
>
> Laurent Pinchart
>

Hi Laurent,

On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Laurent
>
> On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The YamlObject::get() function takes a default value and an optional
> > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > error handling in callers.
> >
> > A better API is possible by dropping the default value and ok flag and
> > returning an std::optional. Not only does it simplify the calls, it also
> > lets callers handle errors through the standard std::optional class
> > instead of the current ad-hoc mechanism.
> >
> > Provide a get() wrapper around std::optional::value_or() to further
> > simplify callers that don't need any specific error handling.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > Tested-by: Naushir Patuck <naush@raspberrypi.com>
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
> > ---
> >  include/libcamera/internal/yaml_parser.h |   9 +-
> >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> >  test/yaml-parser.cpp                     |  71 ++++++------
> >  3 files changed, 96 insertions(+), 122 deletions(-)
> >
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 064cf44381d7..61f2223223a7 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <iterator>
> >  #include <map>
> > +#include <optional>
> >  #include <string>
> >  #include <vector>
> >
> > @@ -165,7 +166,13 @@ public:
> >  #else
> >       template<typename T>
> >  #endif
> > -     T get(const T &defaultValue, bool *ok = nullptr) const;
> > +     std::optional<T> get() const;
> > +
> > +     template<typename T>
> > +     T get(const T &defaultValue) const
> > +     {
> > +             return get<T>().value_or(defaultValue);
> > +     }
> >
I have not seen any test for this specific part (it could be great at
least as an example).

Also, do you see any added value to add it to the getList() function
later on this series?
I find more readable to have something like that:
        std::vector<uint16_t> table = tuningData[prop].getList<uint16_t>({});
than:
        std::vector<uint16_t> table =
tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
in case an empty vector is not expected (and we can do check on size)

> >       DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> >       ListAdapter asList() const { return ListAdapter{ list_ }; }
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 5c45e44e49c3..4299f5abd38a 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -31,12 +31,6 @@ namespace {
> >  /* Empty static YamlObject as a safe result for invalid operations */
> >  static const YamlObject empty;
> >
> > -void setOk(bool *ok, bool result)
> > -{
> > -     if (ok)
> > -             *ok = result;
> > -}
> > -
> >  } /* namespace */
> >
> >  /**
> > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> >  }
> >
> >  /**
> > - * \fn template<typename T> YamlObject::get<T>(
> > - *   const T &defaultValue, bool *ok) const
> > + * \fn template<typename T> YamlObject::get<T>() const
> > + * \brief Parse the YamlObject as a \a T value
> > + *
> > + * This function parses the value of the YamlObject as a \a T object, and
> > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > + * store a \a T value), std::nullopt is returned.
> > + *
> > + * \return The YamlObject value, or std::nullopt if parsing failed
> > + */
> > +
> > +/**
> > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> >   * \brief Parse the YamlObject as a \a T value
> >   * \param[in] defaultValue The default value when failing to parse
> > - * \param[out] ok The result of whether the parse succeeded
> >   *
> >   * This function parses the value of the YamlObject as a \a T object, and
> >   * returns the value. If parsing fails (usually because the YamlObject doesn't
> > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > + * store a \a T value), the \a defaultValue is returned.
> >   *
> > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > - * need to know if parsing succeeded.
> > - *
> > - * \return Value as a bool type
> > + * \return The YamlObject value, or \a defaultValue if parsing failed
> >   */
> >
> >  #ifndef __DOXYGEN__
> >
> >  template<>
> > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > +std::optional<bool> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> > -     if (value_ == "true") {
> > -             setOk(ok, true);
> > +     if (value_ == "true")
> >               return true;
> > -     } else if (value_ == "false") {
> > -             setOk(ok, true);
> > +     else if (value_ == "false")
> >               return false;
> > -     }
> >
> > -     return defaultValue;
> > +     return {};
> >  }
> >
> >  template<>
> > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > +std::optional<int16_t> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (value_ == "")
> > -             return defaultValue;
> > +             return {};
> >
> >       char *end;
> >
> > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> >       if ('\0' != *end || errno == ERANGE ||
> >           value < std::numeric_limits<int16_t>::min() ||
> >           value > std::numeric_limits<int16_t>::max())
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value;
> >  }
> >
> >  template<>
> > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > +std::optional<uint16_t> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (value_ == "")
> > -             return defaultValue;
> > +             return {};
> >
> >       /*
> >        * libyaml parses all scalar values as strings. When a string has
> > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >        */
> >       std::size_t found = value_.find_first_not_of(" \t");
> >       if (found != std::string::npos && value_[found] == '-')
> > -             return defaultValue;
> > +             return {};
> >
> >       char *end;
> >
> > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >       if ('\0' != *end || errno == ERANGE ||
> >           value < std::numeric_limits<uint16_t>::min() ||
> >           value > std::numeric_limits<uint16_t>::max())
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value;
> >  }
> >
> >  template<>
> > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > +std::optional<int32_t> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (value_ == "")
> > -             return defaultValue;
> > +             return {};
> >
> >       char *end;
> >
> > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> >       if ('\0' != *end || errno == ERANGE ||
> >           value < std::numeric_limits<int32_t>::min() ||
> >           value > std::numeric_limits<int32_t>::max())
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value;
> >  }
> >
> >  template<>
> > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > +std::optional<uint32_t> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (value_ == "")
> > -             return defaultValue;
> > +             return {};
> >
> >       /*
> >        * libyaml parses all scalar values as strings. When a string has
> > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >        */
> >       std::size_t found = value_.find_first_not_of(" \t");
> >       if (found != std::string::npos && value_[found] == '-')
> > -             return defaultValue;
> > +             return {};
> >
> >       char *end;
> >
> > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >       if ('\0' != *end || errno == ERANGE ||
> >           value < std::numeric_limits<uint32_t>::min() ||
> >           value > std::numeric_limits<uint32_t>::max())
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value;
> >  }
> >
> >  template<>
> > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > +std::optional<double> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (value_ == "")
> > -             return defaultValue;
> > +             return {};
> >
> >       char *end;
> >
> > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> >       double value = std::strtod(value_.c_str(), &end);
> >
> >       if ('\0' != *end || errno == ERANGE)
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value;
> >  }
> >
> >  template<>
> > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > +std::optional<std::string> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::Value)
> > -             return defaultValue;
> > +             return {};
> >
> > -     setOk(ok, true);
> >       return value_;
> >  }
> >
> >  template<>
> > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > +std::optional<Size> YamlObject::get() const
> >  {
> > -     setOk(ok, false);
> > -
> >       if (type_ != Type::List)
> > -             return defaultValue;
> > +             return {};
> >
> >       if (list_.size() != 2)
> > -             return defaultValue;
> > +             return {};
> >
> > -     /*
> > -      * Add a local variable to validate each dimension in case
> > -      * that ok == nullptr.
> > -      */
> > -     bool valid;
> > -     uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > -     if (!valid)
> > -             return defaultValue;
> > +     auto width = list_[0]->get<uint32_t>();
> > +     if (!width)
> > +             return {};
> >
> > -     uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > -     if (!valid)
> > -             return defaultValue;
> > +     auto height = list_[1]->get<uint32_t>();
> > +     if (!height)
> > +             return {};
> >
> > -     setOk(ok, true);
> > -     return Size(width, height);
> > +     return Size(*width, *height);
> >  }
> >
> >  #endif /* __DOXYGEN__ */
> > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > index 38f848232fa6..ebb654f2ef9c 100644
> > --- a/test/yaml-parser.cpp
> > +++ b/test/yaml-parser.cpp
> > @@ -148,7 +148,6 @@ protected:
> >               }
> >
> >               /* Test string object */
> > -             bool ok;
> >               auto &strObj = (*root)["string"];
> >
> >               if (strObj.isDictionary()) {
> > @@ -161,27 +160,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > +             if (strObj.get<string>().value_or("") != "libcamera") {
> >                       cerr << "String object parse as wrong content" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +             if (strObj.get<int32_t>()) {
> >                       cerr << "String object parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (strObj.get<uint32_t>()) {
> >                       cerr << "String object parse as unsigned integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +             if (strObj.get<double>()) {
> >                       cerr << "String object parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (strObj.get<Size>()) {
> >                       cerr << "String object parse as Size" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -199,27 +198,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > +             if (int32Obj.get<int32_t>().value_or(0) != -100) {
> >                       cerr << "Integer object parse as wrong value" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > +             if (int32Obj.get<string>().value_or("") != "-100") {
> >                       cerr << "Integer object fail to parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > +             if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> >                       cerr << "Integer object fail to parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (int32Obj.get<uint32_t>()) {
> >                       cerr << "Negative integer object parse as unsigned integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (int32Obj.get<Size>()) {
> >                       cerr << "Integer object parse as Size" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -237,27 +236,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > +             if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> >                       cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > +             if (uint32Obj.get<string>().value_or("") != "100") {
> >                       cerr << "Unsigned integer object fail to parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > +             if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> >                       cerr << "Unsigned integer object fail to parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > +             if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> >                       cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (uint32Obj.get<Size>()) {
> >                       cerr << "Unsigned integer object parsed as Size" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -275,27 +274,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > +             if (doubleObj.get<string>().value_or("") != "3.14159") {
> >                       cerr << "Double object fail to parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > +             if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> >                       cerr << "Double object parse as wrong value" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +             if (doubleObj.get<int32_t>()) {
> >                       cerr << "Double object parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (doubleObj.get<uint32_t>()) {
> >                       cerr << "Double object parse as unsigned integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (doubleObj.get<Size>()) {
> >                       cerr << "Double object parse as Size" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -313,27 +312,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (sizeObj.get<string>("", &ok) != "" || ok) {
> > +             if (sizeObj.get<string>()) {
> >                       cerr << "Size object parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +             if (sizeObj.get<double>()) {
> >                       cerr << "Size object parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +             if (sizeObj.get<int32_t>()) {
> >                       cerr << "Size object parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (sizeObj.get<uint32_t>()) {
> >                       cerr << "Size object parse as unsigned integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > +             if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> >                       cerr << "Size object parse as wrong value" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -351,27 +350,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (listObj.get<string>("", &ok) != "" || ok) {
> > +             if (listObj.get<string>()) {
> >                       cerr << "List object parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +             if (listObj.get<double>()) {
> >                       cerr << "List object parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +             if (listObj.get<int32_t>()) {
> >                       cerr << "List object parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (listObj.get<uint32_t>()) {
> >                       cerr << "List object parse as unsigne integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (listObj.get<Size>()) {
> >                       cerr << "String list object parse as Size" << std::endl;
> >                       return TestFail;
> >               }
> > @@ -424,27 +423,27 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             if (dictObj.get<string>("", &ok) != "" || ok) {
> > +             if (dictObj.get<string>()) {
> >                       cerr << "Dictionary object parse as string" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +             if (dictObj.get<double>()) {
> >                       cerr << "Dictionary object parse as double" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +             if (dictObj.get<int32_t>()) {
> >                       cerr << "Dictionary object parse as integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +             if (dictObj.get<uint32_t>()) {
> >                       cerr << "Dictionary object parse as unsigned integer" << std::endl;
> >                       return TestFail;
> >               }
> >
> > -             if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +             if (dictObj.get<Size>()) {
> >                       cerr << "Dictionary object parse as Size" << std::endl;
> >                       return TestFail;
> >               }
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

Reviewed-by: Florian Sylvestre <fsylvestre@baylibre.com>

Hi Florian,

On Fri, Jul 29, 2022 at 09:03:19AM +0200, Florian Sylvestre wrote:
> On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel wrote:
> > On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The YamlObject::get() function takes a default value and an optional
> > > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > > error handling in callers.
> > >
> > > A better API is possible by dropping the default value and ok flag and
> > > returning an std::optional. Not only does it simplify the calls, it also
> > > lets callers handle errors through the standard std::optional class
> > > instead of the current ad-hoc mechanism.
> > >
> > > Provide a get() wrapper around std::optional::value_or() to further
> > > simplify callers that don't need any specific error handling.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > > ---
> > >  include/libcamera/internal/yaml_parser.h |   9 +-
> > >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> > >  test/yaml-parser.cpp                     |  71 ++++++------
> > >  3 files changed, 96 insertions(+), 122 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > > index 064cf44381d7..61f2223223a7 100644
> > > --- a/include/libcamera/internal/yaml_parser.h
> > > +++ b/include/libcamera/internal/yaml_parser.h
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <iterator>
> > >  #include <map>
> > > +#include <optional>
> > >  #include <string>
> > >  #include <vector>
> > >
> > > @@ -165,7 +166,13 @@ public:
> > >  #else
> > >       template<typename T>
> > >  #endif
> > > -     T get(const T &defaultValue, bool *ok = nullptr) const;
> > > +     std::optional<T> get() const;
> > > +
> > > +     template<typename T>
> > > +     T get(const T &defaultValue) const
> > > +     {
> > > +             return get<T>().value_or(defaultValue);
> > > +     }
> 
> I have not seen any test for this specific part (it could be great at
> least as an example).

Good point. I'll add tests.

> Also, do you see any added value to add it to the getList() function
> later on this series?
> I find more readable to have something like that:
>         std::vector<uint16_t> table = tuningData[prop].getList<uint16_t>({});
> than:
>         std::vector<uint16_t> table =
> tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
> in case an empty vector is not expected (and we can do check on size)

Do you mean something along the lines of

	template<typename T>
	std::vector<T> getList(const std::vector<T> &defaultValue);

?

> > >       DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> > >       ListAdapter asList() const { return ListAdapter{ list_ }; }
> > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > > index 5c45e44e49c3..4299f5abd38a 100644
> > > --- a/src/libcamera/yaml_parser.cpp
> > > +++ b/src/libcamera/yaml_parser.cpp
> > > @@ -31,12 +31,6 @@ namespace {
> > >  /* Empty static YamlObject as a safe result for invalid operations */
> > >  static const YamlObject empty;
> > >
> > > -void setOk(bool *ok, bool result)
> > > -{
> > > -     if (ok)
> > > -             *ok = result;
> > > -}
> > > -
> > >  } /* namespace */
> > >
> > >  /**
> > > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> > >  }
> > >
> > >  /**
> > > - * \fn template<typename T> YamlObject::get<T>(
> > > - *   const T &defaultValue, bool *ok) const
> > > + * \fn template<typename T> YamlObject::get<T>() const
> > > + * \brief Parse the YamlObject as a \a T value
> > > + *
> > > + * This function parses the value of the YamlObject as a \a T object, and
> > > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > + * store a \a T value), std::nullopt is returned.
> > > + *
> > > + * \return The YamlObject value, or std::nullopt if parsing failed
> > > + */
> > > +
> > > +/**
> > > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> > >   * \brief Parse the YamlObject as a \a T value
> > >   * \param[in] defaultValue The default value when failing to parse
> > > - * \param[out] ok The result of whether the parse succeeded
> > >   *
> > >   * This function parses the value of the YamlObject as a \a T object, and
> > >   * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > > + * store a \a T value), the \a defaultValue is returned.
> > >   *
> > > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > > - * need to know if parsing succeeded.
> > > - *
> > > - * \return Value as a bool type
> > > + * \return The YamlObject value, or \a defaultValue if parsing failed
> > >   */
> > >
> > >  #ifndef __DOXYGEN__
> > >
> > >  template<>
> > > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > > +std::optional<bool> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     if (value_ == "true") {
> > > -             setOk(ok, true);
> > > +     if (value_ == "true")
> > >               return true;
> > > -     } else if (value_ == "false") {
> > > -             setOk(ok, true);
> > > +     else if (value_ == "false")
> > >               return false;
> > > -     }
> > >
> > > -     return defaultValue;
> > > +     return {};
> > >  }
> > >
> > >  template<>
> > > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > +std::optional<int16_t> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (value_ == "")
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       char *end;
> > >
> > > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > >       if ('\0' != *end || errno == ERANGE ||
> > >           value < std::numeric_limits<int16_t>::min() ||
> > >           value > std::numeric_limits<int16_t>::max())
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value;
> > >  }
> > >
> > >  template<>
> > > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > +std::optional<uint16_t> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (value_ == "")
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       /*
> > >        * libyaml parses all scalar values as strings. When a string has
> > > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > >        */
> > >       std::size_t found = value_.find_first_not_of(" \t");
> > >       if (found != std::string::npos && value_[found] == '-')
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       char *end;
> > >
> > > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > >       if ('\0' != *end || errno == ERANGE ||
> > >           value < std::numeric_limits<uint16_t>::min() ||
> > >           value > std::numeric_limits<uint16_t>::max())
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value;
> > >  }
> > >
> > >  template<>
> > > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > +std::optional<int32_t> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (value_ == "")
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       char *end;
> > >
> > > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > >       if ('\0' != *end || errno == ERANGE ||
> > >           value < std::numeric_limits<int32_t>::min() ||
> > >           value > std::numeric_limits<int32_t>::max())
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value;
> > >  }
> > >
> > >  template<>
> > > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > +std::optional<uint32_t> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (value_ == "")
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       /*
> > >        * libyaml parses all scalar values as strings. When a string has
> > > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > >        */
> > >       std::size_t found = value_.find_first_not_of(" \t");
> > >       if (found != std::string::npos && value_[found] == '-')
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       char *end;
> > >
> > > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > >       if ('\0' != *end || errno == ERANGE ||
> > >           value < std::numeric_limits<uint32_t>::min() ||
> > >           value > std::numeric_limits<uint32_t>::max())
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value;
> > >  }
> > >
> > >  template<>
> > > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > > +std::optional<double> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (value_ == "")
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       char *end;
> > >
> > > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> > >       double value = std::strtod(value_.c_str(), &end);
> > >
> > >       if ('\0' != *end || errno == ERANGE)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value;
> > >  }
> > >
> > >  template<>
> > > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > > +std::optional<std::string> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::Value)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > >       return value_;
> > >  }
> > >
> > >  template<>
> > > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > > +std::optional<Size> YamlObject::get() const
> > >  {
> > > -     setOk(ok, false);
> > > -
> > >       if (type_ != Type::List)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > >       if (list_.size() != 2)
> > > -             return defaultValue;
> > > +             return {};
> > >
> > > -     /*
> > > -      * Add a local variable to validate each dimension in case
> > > -      * that ok == nullptr.
> > > -      */
> > > -     bool valid;
> > > -     uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > > -     if (!valid)
> > > -             return defaultValue;
> > > +     auto width = list_[0]->get<uint32_t>();
> > > +     if (!width)
> > > +             return {};
> > >
> > > -     uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > > -     if (!valid)
> > > -             return defaultValue;
> > > +     auto height = list_[1]->get<uint32_t>();
> > > +     if (!height)
> > > +             return {};
> > >
> > > -     setOk(ok, true);
> > > -     return Size(width, height);
> > > +     return Size(*width, *height);
> > >  }
> > >
> > >  #endif /* __DOXYGEN__ */
> > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > index 38f848232fa6..ebb654f2ef9c 100644
> > > --- a/test/yaml-parser.cpp
> > > +++ b/test/yaml-parser.cpp
> > > @@ -148,7 +148,6 @@ protected:
> > >               }
> > >
> > >               /* Test string object */
> > > -             bool ok;
> > >               auto &strObj = (*root)["string"];
> > >
> > >               if (strObj.isDictionary()) {
> > > @@ -161,27 +160,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > > +             if (strObj.get<string>().value_or("") != "libcamera") {
> > >                       cerr << "String object parse as wrong content" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > +             if (strObj.get<int32_t>()) {
> > >                       cerr << "String object parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (strObj.get<uint32_t>()) {
> > >                       cerr << "String object parse as unsigned integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > +             if (strObj.get<double>()) {
> > >                       cerr << "String object parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (strObj.get<Size>()) {
> > >                       cerr << "String object parse as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -199,27 +198,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > > +             if (int32Obj.get<int32_t>().value_or(0) != -100) {
> > >                       cerr << "Integer object parse as wrong value" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > > +             if (int32Obj.get<string>().value_or("") != "-100") {
> > >                       cerr << "Integer object fail to parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > > +             if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> > >                       cerr << "Integer object fail to parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (int32Obj.get<uint32_t>()) {
> > >                       cerr << "Negative integer object parse as unsigned integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (int32Obj.get<Size>()) {
> > >                       cerr << "Integer object parse as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -237,27 +236,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > > +             if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> > >                       cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > > +             if (uint32Obj.get<string>().value_or("") != "100") {
> > >                       cerr << "Unsigned integer object fail to parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > > +             if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> > >                       cerr << "Unsigned integer object fail to parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > > +             if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> > >                       cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (uint32Obj.get<Size>()) {
> > >                       cerr << "Unsigned integer object parsed as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -275,27 +274,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > > +             if (doubleObj.get<string>().value_or("") != "3.14159") {
> > >                       cerr << "Double object fail to parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > > +             if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> > >                       cerr << "Double object parse as wrong value" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > +             if (doubleObj.get<int32_t>()) {
> > >                       cerr << "Double object parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (doubleObj.get<uint32_t>()) {
> > >                       cerr << "Double object parse as unsigned integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (doubleObj.get<Size>()) {
> > >                       cerr << "Double object parse as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -313,27 +312,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (sizeObj.get<string>("", &ok) != "" || ok) {
> > > +             if (sizeObj.get<string>()) {
> > >                       cerr << "Size object parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > +             if (sizeObj.get<double>()) {
> > >                       cerr << "Size object parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > +             if (sizeObj.get<int32_t>()) {
> > >                       cerr << "Size object parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (sizeObj.get<uint32_t>()) {
> > >                       cerr << "Size object parse as unsigned integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > > +             if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> > >                       cerr << "Size object parse as wrong value" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -351,27 +350,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (listObj.get<string>("", &ok) != "" || ok) {
> > > +             if (listObj.get<string>()) {
> > >                       cerr << "List object parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > +             if (listObj.get<double>()) {
> > >                       cerr << "List object parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > +             if (listObj.get<int32_t>()) {
> > >                       cerr << "List object parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (listObj.get<uint32_t>()) {
> > >                       cerr << "List object parse as unsigne integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (listObj.get<Size>()) {
> > >                       cerr << "String list object parse as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> > > @@ -424,27 +423,27 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (dictObj.get<string>("", &ok) != "" || ok) {
> > > +             if (dictObj.get<string>()) {
> > >                       cerr << "Dictionary object parse as string" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > +             if (dictObj.get<double>()) {
> > >                       cerr << "Dictionary object parse as double" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > +             if (dictObj.get<int32_t>()) {
> > >                       cerr << "Dictionary object parse as integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > +             if (dictObj.get<uint32_t>()) {
> > >                       cerr << "Dictionary object parse as unsigned integer" << std::endl;
> > >                       return TestFail;
> > >               }
> > >
> > > -             if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > +             if (dictObj.get<Size>()) {
> > >                       cerr << "Dictionary object parse as Size" << std::endl;
> > >                       return TestFail;
> > >               }
> 
> Reviewed-by: Florian Sylvestre <fsylvestre@baylibre.com>

Hello Laurent,

On Sat, 30 Jul 2022 at 15:20, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Florian,
>
> On Fri, Jul 29, 2022 at 09:03:19AM +0200, Florian Sylvestre wrote:
> > On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel wrote:
> > > On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > The YamlObject::get() function takes a default value and an optional
> > > > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > > > error handling in callers.
> > > >
> > > > A better API is possible by dropping the default value and ok flag and
> > > > returning an std::optional. Not only does it simplify the calls, it also
> > > > lets callers handle errors through the standard std::optional class
> > > > instead of the current ad-hoc mechanism.
> > > >
> > > > Provide a get() wrapper around std::optional::value_or() to further
> > > > simplify callers that don't need any specific error handling.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > > ---
> > > >  include/libcamera/internal/yaml_parser.h |   9 +-
> > > >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> > > >  test/yaml-parser.cpp                     |  71 ++++++------
> > > >  3 files changed, 96 insertions(+), 122 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > > > index 064cf44381d7..61f2223223a7 100644
> > > > --- a/include/libcamera/internal/yaml_parser.h
> > > > +++ b/include/libcamera/internal/yaml_parser.h
> > > > @@ -9,6 +9,7 @@
> > > >
> > > >  #include <iterator>
> > > >  #include <map>
> > > > +#include <optional>
> > > >  #include <string>
> > > >  #include <vector>
> > > >
> > > > @@ -165,7 +166,13 @@ public:
> > > >  #else
> > > >       template<typename T>
> > > >  #endif
> > > > -     T get(const T &defaultValue, bool *ok = nullptr) const;
> > > > +     std::optional<T> get() const;
> > > > +
> > > > +     template<typename T>
> > > > +     T get(const T &defaultValue) const
> > > > +     {
> > > > +             return get<T>().value_or(defaultValue);
> > > > +     }
> >
> > I have not seen any test for this specific part (it could be great at
> > least as an example).
>
> Good point. I'll add tests.
>
> > Also, do you see any added value to add it to the getList() function
> > later on this series?
> > I find more readable to have something like that:
> >         std::vector<uint16_t> table = tuningData[prop].getList<uint16_t>({});
> > than:
> >         std::vector<uint16_t> table =
> > tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
> > in case an empty vector is not expected (and we can do check on size)
>
> Do you mean something along the lines of
>
>         template<typename T>
>         std::vector<T> getList(const std::vector<T> &defaultValue);
>
> ?
I was thinking of adding:
        template<typename T>
        std::vector<T> getList(const std::vector<T> &defaultValue) const
        {
                return getList<T>().value_or(defaultValue);
        }

To allow the following call:
        std::vector<uint16_t> x = tuningData["x"].getList<uint16_t>({});
that seems more readable than:
        std::vector<uint16_t> x =
tuningData["x"].getList<uint16_t>().value_or(utils::defopt);

> > > >       DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> > > >       ListAdapter asList() const { return ListAdapter{ list_ }; }
> > > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > > > index 5c45e44e49c3..4299f5abd38a 100644
> > > > --- a/src/libcamera/yaml_parser.cpp
> > > > +++ b/src/libcamera/yaml_parser.cpp
> > > > @@ -31,12 +31,6 @@ namespace {
> > > >  /* Empty static YamlObject as a safe result for invalid operations */
> > > >  static const YamlObject empty;
> > > >
> > > > -void setOk(bool *ok, bool result)
> > > > -{
> > > > -     if (ok)
> > > > -             *ok = result;
> > > > -}
> > > > -
> > > >  } /* namespace */
> > > >
> > > >  /**
> > > > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> > > >  }
> > > >
> > > >  /**
> > > > - * \fn template<typename T> YamlObject::get<T>(
> > > > - *   const T &defaultValue, bool *ok) const
> > > > + * \fn template<typename T> YamlObject::get<T>() const
> > > > + * \brief Parse the YamlObject as a \a T value
> > > > + *
> > > > + * This function parses the value of the YamlObject as a \a T object, and
> > > > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > > + * store a \a T value), std::nullopt is returned.
> > > > + *
> > > > + * \return The YamlObject value, or std::nullopt if parsing failed
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> > > >   * \brief Parse the YamlObject as a \a T value
> > > >   * \param[in] defaultValue The default value when failing to parse
> > > > - * \param[out] ok The result of whether the parse succeeded
> > > >   *
> > > >   * This function parses the value of the YamlObject as a \a T object, and
> > > >   * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > > > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > > > + * store a \a T value), the \a defaultValue is returned.
> > > >   *
> > > > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > > > - * need to know if parsing succeeded.
> > > > - *
> > > > - * \return Value as a bool type
> > > > + * \return The YamlObject value, or \a defaultValue if parsing failed
> > > >   */
> > > >
> > > >  #ifndef __DOXYGEN__
> > > >
> > > >  template<>
> > > > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > > > +std::optional<bool> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     if (value_ == "true") {
> > > > -             setOk(ok, true);
> > > > +     if (value_ == "true")
> > > >               return true;
> > > > -     } else if (value_ == "false") {
> > > > -             setOk(ok, true);
> > > > +     else if (value_ == "false")
> > > >               return false;
> > > > -     }
> > > >
> > > > -     return defaultValue;
> > > > +     return {};
> > > >  }
> > > >
> > > >  template<>
> > > > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > > +std::optional<int16_t> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (value_ == "")
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       char *end;
> > > >
> > > > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > >       if ('\0' != *end || errno == ERANGE ||
> > > >           value < std::numeric_limits<int16_t>::min() ||
> > > >           value > std::numeric_limits<int16_t>::max())
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value;
> > > >  }
> > > >
> > > >  template<>
> > > > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > > +std::optional<uint16_t> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (value_ == "")
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       /*
> > > >        * libyaml parses all scalar values as strings. When a string has
> > > > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > >        */
> > > >       std::size_t found = value_.find_first_not_of(" \t");
> > > >       if (found != std::string::npos && value_[found] == '-')
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       char *end;
> > > >
> > > > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > >       if ('\0' != *end || errno == ERANGE ||
> > > >           value < std::numeric_limits<uint16_t>::min() ||
> > > >           value > std::numeric_limits<uint16_t>::max())
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value;
> > > >  }
> > > >
> > > >  template<>
> > > > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > > +std::optional<int32_t> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (value_ == "")
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       char *end;
> > > >
> > > > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > >       if ('\0' != *end || errno == ERANGE ||
> > > >           value < std::numeric_limits<int32_t>::min() ||
> > > >           value > std::numeric_limits<int32_t>::max())
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value;
> > > >  }
> > > >
> > > >  template<>
> > > > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > > +std::optional<uint32_t> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (value_ == "")
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       /*
> > > >        * libyaml parses all scalar values as strings. When a string has
> > > > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > >        */
> > > >       std::size_t found = value_.find_first_not_of(" \t");
> > > >       if (found != std::string::npos && value_[found] == '-')
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       char *end;
> > > >
> > > > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > >       if ('\0' != *end || errno == ERANGE ||
> > > >           value < std::numeric_limits<uint32_t>::min() ||
> > > >           value > std::numeric_limits<uint32_t>::max())
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value;
> > > >  }
> > > >
> > > >  template<>
> > > > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > > > +std::optional<double> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (value_ == "")
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       char *end;
> > > >
> > > > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> > > >       double value = std::strtod(value_.c_str(), &end);
> > > >
> > > >       if ('\0' != *end || errno == ERANGE)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value;
> > > >  }
> > > >
> > > >  template<>
> > > > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > > > +std::optional<std::string> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::Value)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > >       return value_;
> > > >  }
> > > >
> > > >  template<>
> > > > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > > > +std::optional<Size> YamlObject::get() const
> > > >  {
> > > > -     setOk(ok, false);
> > > > -
> > > >       if (type_ != Type::List)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > >       if (list_.size() != 2)
> > > > -             return defaultValue;
> > > > +             return {};
> > > >
> > > > -     /*
> > > > -      * Add a local variable to validate each dimension in case
> > > > -      * that ok == nullptr.
> > > > -      */
> > > > -     bool valid;
> > > > -     uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > > > -     if (!valid)
> > > > -             return defaultValue;
> > > > +     auto width = list_[0]->get<uint32_t>();
> > > > +     if (!width)
> > > > +             return {};
> > > >
> > > > -     uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > > > -     if (!valid)
> > > > -             return defaultValue;
> > > > +     auto height = list_[1]->get<uint32_t>();
> > > > +     if (!height)
> > > > +             return {};
> > > >
> > > > -     setOk(ok, true);
> > > > -     return Size(width, height);
> > > > +     return Size(*width, *height);
> > > >  }
> > > >
> > > >  #endif /* __DOXYGEN__ */
> > > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > > index 38f848232fa6..ebb654f2ef9c 100644
> > > > --- a/test/yaml-parser.cpp
> > > > +++ b/test/yaml-parser.cpp
> > > > @@ -148,7 +148,6 @@ protected:
> > > >               }
> > > >
> > > >               /* Test string object */
> > > > -             bool ok;
> > > >               auto &strObj = (*root)["string"];
> > > >
> > > >               if (strObj.isDictionary()) {
> > > > @@ -161,27 +160,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > > > +             if (strObj.get<string>().value_or("") != "libcamera") {
> > > >                       cerr << "String object parse as wrong content" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > +             if (strObj.get<int32_t>()) {
> > > >                       cerr << "String object parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (strObj.get<uint32_t>()) {
> > > >                       cerr << "String object parse as unsigned integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > +             if (strObj.get<double>()) {
> > > >                       cerr << "String object parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (strObj.get<Size>()) {
> > > >                       cerr << "String object parse as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -199,27 +198,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > > > +             if (int32Obj.get<int32_t>().value_or(0) != -100) {
> > > >                       cerr << "Integer object parse as wrong value" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > > > +             if (int32Obj.get<string>().value_or("") != "-100") {
> > > >                       cerr << "Integer object fail to parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > > > +             if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> > > >                       cerr << "Integer object fail to parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (int32Obj.get<uint32_t>()) {
> > > >                       cerr << "Negative integer object parse as unsigned integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (int32Obj.get<Size>()) {
> > > >                       cerr << "Integer object parse as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -237,27 +236,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > > > +             if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> > > >                       cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > > > +             if (uint32Obj.get<string>().value_or("") != "100") {
> > > >                       cerr << "Unsigned integer object fail to parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > > > +             if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> > > >                       cerr << "Unsigned integer object fail to parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > > > +             if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> > > >                       cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (uint32Obj.get<Size>()) {
> > > >                       cerr << "Unsigned integer object parsed as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -275,27 +274,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > > > +             if (doubleObj.get<string>().value_or("") != "3.14159") {
> > > >                       cerr << "Double object fail to parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > > > +             if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> > > >                       cerr << "Double object parse as wrong value" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > +             if (doubleObj.get<int32_t>()) {
> > > >                       cerr << "Double object parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (doubleObj.get<uint32_t>()) {
> > > >                       cerr << "Double object parse as unsigned integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (doubleObj.get<Size>()) {
> > > >                       cerr << "Double object parse as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -313,27 +312,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (sizeObj.get<string>("", &ok) != "" || ok) {
> > > > +             if (sizeObj.get<string>()) {
> > > >                       cerr << "Size object parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > +             if (sizeObj.get<double>()) {
> > > >                       cerr << "Size object parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > +             if (sizeObj.get<int32_t>()) {
> > > >                       cerr << "Size object parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (sizeObj.get<uint32_t>()) {
> > > >                       cerr << "Size object parse as unsigned integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > > > +             if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> > > >                       cerr << "Size object parse as wrong value" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -351,27 +350,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (listObj.get<string>("", &ok) != "" || ok) {
> > > > +             if (listObj.get<string>()) {
> > > >                       cerr << "List object parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > +             if (listObj.get<double>()) {
> > > >                       cerr << "List object parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > +             if (listObj.get<int32_t>()) {
> > > >                       cerr << "List object parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (listObj.get<uint32_t>()) {
> > > >                       cerr << "List object parse as unsigne integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (listObj.get<Size>()) {
> > > >                       cerr << "String list object parse as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > > @@ -424,27 +423,27 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (dictObj.get<string>("", &ok) != "" || ok) {
> > > > +             if (dictObj.get<string>()) {
> > > >                       cerr << "Dictionary object parse as string" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > +             if (dictObj.get<double>()) {
> > > >                       cerr << "Dictionary object parse as double" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > +             if (dictObj.get<int32_t>()) {
> > > >                       cerr << "Dictionary object parse as integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > +             if (dictObj.get<uint32_t>()) {
> > > >                       cerr << "Dictionary object parse as unsigned integer" << std::endl;
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > +             if (dictObj.get<Size>()) {
> > > >                       cerr << "Dictionary object parse as Size" << std::endl;
> > > >                       return TestFail;
> > > >               }
> >
> > Reviewed-by: Florian Sylvestre <fsylvestre@baylibre.com>
>
> --
> Regards,
>
> Laurent Pinchart

Hi Florian,

On Mon, Aug 01, 2022 at 10:32:53PM +0200, Florian Sylvestre wrote:
> On Sat, 30 Jul 2022 at 15:20, Laurent Pinchart wrote:
> > On Fri, Jul 29, 2022 at 09:03:19AM +0200, Florian Sylvestre wrote:
> > > On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel wrote:
> > > > On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > > The YamlObject::get() function takes a default value and an optional
> > > > > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > > > > error handling in callers.
> > > > >
> > > > > A better API is possible by dropping the default value and ok flag and
> > > > > returning an std::optional. Not only does it simplify the calls, it also
> > > > > lets callers handle errors through the standard std::optional class
> > > > > instead of the current ad-hoc mechanism.
> > > > >
> > > > > Provide a get() wrapper around std::optional::value_or() to further
> > > > > simplify callers that don't need any specific error handling.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > > ---
> > > > >  include/libcamera/internal/yaml_parser.h |   9 +-
> > > > >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> > > > >  test/yaml-parser.cpp                     |  71 ++++++------
> > > > >  3 files changed, 96 insertions(+), 122 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > > > > index 064cf44381d7..61f2223223a7 100644
> > > > > --- a/include/libcamera/internal/yaml_parser.h
> > > > > +++ b/include/libcamera/internal/yaml_parser.h
> > > > > @@ -9,6 +9,7 @@
> > > > >
> > > > >  #include <iterator>
> > > > >  #include <map>
> > > > > +#include <optional>
> > > > >  #include <string>
> > > > >  #include <vector>
> > > > >
> > > > > @@ -165,7 +166,13 @@ public:
> > > > >  #else
> > > > >       template<typename T>
> > > > >  #endif
> > > > > -     T get(const T &defaultValue, bool *ok = nullptr) const;
> > > > > +     std::optional<T> get() const;
> > > > > +
> > > > > +     template<typename T>
> > > > > +     T get(const T &defaultValue) const
> > > > > +     {
> > > > > +             return get<T>().value_or(defaultValue);
> > > > > +     }
> > >
> > > I have not seen any test for this specific part (it could be great at
> > > least as an example).
> >
> > Good point. I'll add tests.

Done and merged btw.

> > > Also, do you see any added value to add it to the getList() function
> > > later on this series?
> > > I find more readable to have something like that:
> > >         std::vector<uint16_t> table = tuningData[prop].getList<uint16_t>({});
> > > than:
> > >         std::vector<uint16_t> table =
> > > tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
> > > in case an empty vector is not expected (and we can do check on size)
> >
> > Do you mean something along the lines of
> >
> >         template<typename T>
> >         std::vector<T> getList(const std::vector<T> &defaultValue);
> >
> > ?
> 
> I was thinking of adding:
>         template<typename T>
>         std::vector<T> getList(const std::vector<T> &defaultValue) const
>         {
>                 return getList<T>().value_or(defaultValue);
>         }
> 
> To allow the following call:
>         std::vector<uint16_t> x = tuningData["x"].getList<uint16_t>({});
> that seems more readable than:
>         std::vector<uint16_t> x = tuningData["x"].getList<uint16_t>().value_or(utils::defopt);

utils::defopt is meant as an optimization to avoid the construction of a
default value passed to the function in the case the list is not empty.
It's probably not that costly in this specific case for an empty
std::vector, but I don't like this quite ad-hoc API to be honest. It
pushes callers towards inefficient constructs.

> > > > >       DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> > > > >       ListAdapter asList() const { return ListAdapter{ list_ }; }
> > > > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > > > > index 5c45e44e49c3..4299f5abd38a 100644
> > > > > --- a/src/libcamera/yaml_parser.cpp
> > > > > +++ b/src/libcamera/yaml_parser.cpp
> > > > > @@ -31,12 +31,6 @@ namespace {
> > > > >  /* Empty static YamlObject as a safe result for invalid operations */
> > > > >  static const YamlObject empty;
> > > > >
> > > > > -void setOk(bool *ok, bool result)
> > > > > -{
> > > > > -     if (ok)
> > > > > -             *ok = result;
> > > > > -}
> > > > > -
> > > > >  } /* namespace */
> > > > >
> > > > >  /**
> > > > > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * \fn template<typename T> YamlObject::get<T>(
> > > > > - *   const T &defaultValue, bool *ok) const
> > > > > + * \fn template<typename T> YamlObject::get<T>() const
> > > > > + * \brief Parse the YamlObject as a \a T value
> > > > > + *
> > > > > + * This function parses the value of the YamlObject as a \a T object, and
> > > > > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > > > + * store a \a T value), std::nullopt is returned.
> > > > > + *
> > > > > + * \return The YamlObject value, or std::nullopt if parsing failed
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> > > > >   * \brief Parse the YamlObject as a \a T value
> > > > >   * \param[in] defaultValue The default value when failing to parse
> > > > > - * \param[out] ok The result of whether the parse succeeded
> > > > >   *
> > > > >   * This function parses the value of the YamlObject as a \a T object, and
> > > > >   * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > > > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > > > > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > > > > + * store a \a T value), the \a defaultValue is returned.
> > > > >   *
> > > > > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > > > > - * need to know if parsing succeeded.
> > > > > - *
> > > > > - * \return Value as a bool type
> > > > > + * \return The YamlObject value, or \a defaultValue if parsing failed
> > > > >   */
> > > > >
> > > > >  #ifndef __DOXYGEN__
> > > > >
> > > > >  template<>
> > > > > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > > > > +std::optional<bool> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     if (value_ == "true") {
> > > > > -             setOk(ok, true);
> > > > > +     if (value_ == "true")
> > > > >               return true;
> > > > > -     } else if (value_ == "false") {
> > > > > -             setOk(ok, true);
> > > > > +     else if (value_ == "false")
> > > > >               return false;
> > > > > -     }
> > > > >
> > > > > -     return defaultValue;
> > > > > +     return {};
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > > > +std::optional<int16_t> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (value_ == "")
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       char *end;
> > > > >
> > > > > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > > >       if ('\0' != *end || errno == ERANGE ||
> > > > >           value < std::numeric_limits<int16_t>::min() ||
> > > > >           value > std::numeric_limits<int16_t>::max())
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > > > +std::optional<uint16_t> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (value_ == "")
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       /*
> > > > >        * libyaml parses all scalar values as strings. When a string has
> > > > > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > > >        */
> > > > >       std::size_t found = value_.find_first_not_of(" \t");
> > > > >       if (found != std::string::npos && value_[found] == '-')
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       char *end;
> > > > >
> > > > > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > > >       if ('\0' != *end || errno == ERANGE ||
> > > > >           value < std::numeric_limits<uint16_t>::min() ||
> > > > >           value > std::numeric_limits<uint16_t>::max())
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > > > +std::optional<int32_t> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (value_ == "")
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       char *end;
> > > > >
> > > > > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > > >       if ('\0' != *end || errno == ERANGE ||
> > > > >           value < std::numeric_limits<int32_t>::min() ||
> > > > >           value > std::numeric_limits<int32_t>::max())
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > > > +std::optional<uint32_t> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (value_ == "")
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       /*
> > > > >        * libyaml parses all scalar values as strings. When a string has
> > > > > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > > >        */
> > > > >       std::size_t found = value_.find_first_not_of(" \t");
> > > > >       if (found != std::string::npos && value_[found] == '-')
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       char *end;
> > > > >
> > > > > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > > >       if ('\0' != *end || errno == ERANGE ||
> > > > >           value < std::numeric_limits<uint32_t>::min() ||
> > > > >           value > std::numeric_limits<uint32_t>::max())
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > > > > +std::optional<double> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (value_ == "")
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       char *end;
> > > > >
> > > > > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> > > > >       double value = std::strtod(value_.c_str(), &end);
> > > > >
> > > > >       if ('\0' != *end || errno == ERANGE)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > > > > +std::optional<std::string> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::Value)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > >       return value_;
> > > > >  }
> > > > >
> > > > >  template<>
> > > > > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > > > > +std::optional<Size> YamlObject::get() const
> > > > >  {
> > > > > -     setOk(ok, false);
> > > > > -
> > > > >       if (type_ != Type::List)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > >       if (list_.size() != 2)
> > > > > -             return defaultValue;
> > > > > +             return {};
> > > > >
> > > > > -     /*
> > > > > -      * Add a local variable to validate each dimension in case
> > > > > -      * that ok == nullptr.
> > > > > -      */
> > > > > -     bool valid;
> > > > > -     uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > > > > -     if (!valid)
> > > > > -             return defaultValue;
> > > > > +     auto width = list_[0]->get<uint32_t>();
> > > > > +     if (!width)
> > > > > +             return {};
> > > > >
> > > > > -     uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > > > > -     if (!valid)
> > > > > -             return defaultValue;
> > > > > +     auto height = list_[1]->get<uint32_t>();
> > > > > +     if (!height)
> > > > > +             return {};
> > > > >
> > > > > -     setOk(ok, true);
> > > > > -     return Size(width, height);
> > > > > +     return Size(*width, *height);
> > > > >  }
> > > > >
> > > > >  #endif /* __DOXYGEN__ */
> > > > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > > > index 38f848232fa6..ebb654f2ef9c 100644
> > > > > --- a/test/yaml-parser.cpp
> > > > > +++ b/test/yaml-parser.cpp
> > > > > @@ -148,7 +148,6 @@ protected:
> > > > >               }
> > > > >
> > > > >               /* Test string object */
> > > > > -             bool ok;
> > > > >               auto &strObj = (*root)["string"];
> > > > >
> > > > >               if (strObj.isDictionary()) {
> > > > > @@ -161,27 +160,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > > > > +             if (strObj.get<string>().value_or("") != "libcamera") {
> > > > >                       cerr << "String object parse as wrong content" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > > +             if (strObj.get<int32_t>()) {
> > > > >                       cerr << "String object parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (strObj.get<uint32_t>()) {
> > > > >                       cerr << "String object parse as unsigned integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > > +             if (strObj.get<double>()) {
> > > > >                       cerr << "String object parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (strObj.get<Size>()) {
> > > > >                       cerr << "String object parse as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -199,27 +198,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > > > > +             if (int32Obj.get<int32_t>().value_or(0) != -100) {
> > > > >                       cerr << "Integer object parse as wrong value" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > > > > +             if (int32Obj.get<string>().value_or("") != "-100") {
> > > > >                       cerr << "Integer object fail to parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > > > > +             if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> > > > >                       cerr << "Integer object fail to parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (int32Obj.get<uint32_t>()) {
> > > > >                       cerr << "Negative integer object parse as unsigned integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (int32Obj.get<Size>()) {
> > > > >                       cerr << "Integer object parse as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -237,27 +236,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > > > > +             if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> > > > >                       cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > > > > +             if (uint32Obj.get<string>().value_or("") != "100") {
> > > > >                       cerr << "Unsigned integer object fail to parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > > > > +             if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> > > > >                       cerr << "Unsigned integer object fail to parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > > > > +             if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> > > > >                       cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (uint32Obj.get<Size>()) {
> > > > >                       cerr << "Unsigned integer object parsed as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -275,27 +274,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > > > > +             if (doubleObj.get<string>().value_or("") != "3.14159") {
> > > > >                       cerr << "Double object fail to parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > > > > +             if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> > > > >                       cerr << "Double object parse as wrong value" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > > +             if (doubleObj.get<int32_t>()) {
> > > > >                       cerr << "Double object parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (doubleObj.get<uint32_t>()) {
> > > > >                       cerr << "Double object parse as unsigned integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (doubleObj.get<Size>()) {
> > > > >                       cerr << "Double object parse as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -313,27 +312,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (sizeObj.get<string>("", &ok) != "" || ok) {
> > > > > +             if (sizeObj.get<string>()) {
> > > > >                       cerr << "Size object parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > > +             if (sizeObj.get<double>()) {
> > > > >                       cerr << "Size object parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > > +             if (sizeObj.get<int32_t>()) {
> > > > >                       cerr << "Size object parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (sizeObj.get<uint32_t>()) {
> > > > >                       cerr << "Size object parse as unsigned integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > > > > +             if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> > > > >                       cerr << "Size object parse as wrong value" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -351,27 +350,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (listObj.get<string>("", &ok) != "" || ok) {
> > > > > +             if (listObj.get<string>()) {
> > > > >                       cerr << "List object parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > > +             if (listObj.get<double>()) {
> > > > >                       cerr << "List object parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > > +             if (listObj.get<int32_t>()) {
> > > > >                       cerr << "List object parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (listObj.get<uint32_t>()) {
> > > > >                       cerr << "List object parse as unsigne integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (listObj.get<Size>()) {
> > > > >                       cerr << "String list object parse as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > > @@ -424,27 +423,27 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (dictObj.get<string>("", &ok) != "" || ok) {
> > > > > +             if (dictObj.get<string>()) {
> > > > >                       cerr << "Dictionary object parse as string" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > > > +             if (dictObj.get<double>()) {
> > > > >                       cerr << "Dictionary object parse as double" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > > > +             if (dictObj.get<int32_t>()) {
> > > > >                       cerr << "Dictionary object parse as integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > > > +             if (dictObj.get<uint32_t>()) {
> > > > >                       cerr << "Dictionary object parse as unsigned integer" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > > > +             if (dictObj.get<Size>()) {
> > > > >                       cerr << "Dictionary object parse as Size" << std::endl;
> > > > >                       return TestFail;
> > > > >               }
> > >
> > > Reviewed-by: Florian Sylvestre <fsylvestre@baylibre.com>

Dear Laurent,

The "return {};" in this patch cause "maybe-uninitialized" warnings in
my CI pipeline:

../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp>
libcamera::YamlObject::get() const [with T = short unsigned int;
std::enable_if_t<(((((((is_same_v<bool, T> || is_same_v<double, T>) ||
is_same_v<short int, T>) || is_same_v<short unsigned int, T>) ||
is_same_v<int, T>) || is_same_v<unsigned int, T>) ||
is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >, T>) || is_same_v<libcamera::Size, T>)>*
<anonymous> = 0]’:
../src/libcamera/yaml_parser.cpp:184:11: error: ‘<anonymous>’ may be
used uninitialized in this function [-Werror=maybe-uninitialized]
  184 |   return {};
      |           ^

... and a couple more in "yaml_parser.cpp".

Since your intention is here to notify the user of invalid values,
wouldn't it be better to "return nullptr;" instead?

Best,
Christian


Am 28.07.22 um 00:21 schrieb Laurent Pinchart via libcamera-devel:
> The YamlObject::get() function takes a default value and an optional
> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> error handling in callers.
>
> A better API is possible by dropping the default value and ok flag and
> returning an std::optional. Not only does it simplify the calls, it also
> lets callers handle errors through the standard std::optional class
> instead of the current ad-hoc mechanism.
>
> Provide a get() wrapper around std::optional::value_or() to further
> simplify callers that don't need any specific error handling.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/yaml_parser.h |   9 +-
>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
>  test/yaml-parser.cpp                     |  71 ++++++------
>  3 files changed, 96 insertions(+), 122 deletions(-)
>
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 064cf44381d7..61f2223223a7 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -9,6 +9,7 @@
>
>  #include <iterator>
>  #include <map>
> +#include <optional>
>  #include <string>
>  #include <vector>
>
> @@ -165,7 +166,13 @@ public:
>  #else
>  	template<typename T>
>  #endif
> -	T get(const T &defaultValue, bool *ok = nullptr) const;
> +	std::optional<T> get() const;
> +
> +	template<typename T>
> +	T get(const T &defaultValue) const
> +	{
> +		return get<T>().value_or(defaultValue);
> +	}
>
>  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
>  	ListAdapter asList() const { return ListAdapter{ list_ }; }
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 5c45e44e49c3..4299f5abd38a 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -31,12 +31,6 @@ namespace {
>  /* Empty static YamlObject as a safe result for invalid operations */
>  static const YamlObject empty;
>
> -void setOk(bool *ok, bool result)
> -{
> -	if (ok)
> -		*ok = result;
> -}
> -
>  } /* namespace */
>
>  /**
> @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
>  }
>
>  /**
> - * \fn template<typename T> YamlObject::get<T>(
> - *	const T &defaultValue, bool *ok) const
> + * \fn template<typename T> YamlObject::get<T>() const
> + * \brief Parse the YamlObject as a \a T value
> + *
> + * This function parses the value of the YamlObject as a \a T object, and
> + * returns the value. If parsing fails (usually because the YamlObject doesn't
> + * store a \a T value), std::nullopt is returned.
> + *
> + * \return The YamlObject value, or std::nullopt if parsing failed
> + */
> +
> +/**
> + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
>   * \brief Parse the YamlObject as a \a T value
>   * \param[in] defaultValue The default value when failing to parse
> - * \param[out] ok The result of whether the parse succeeded
>   *
>   * This function parses the value of the YamlObject as a \a T object, and
>   * returns the value. If parsing fails (usually because the YamlObject doesn't
> - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> + * store a \a T value), the \a defaultValue is returned.
>   *
> - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> - * need to know if parsing succeeded.
> - *
> - * \return Value as a bool type
> + * \return The YamlObject value, or \a defaultValue if parsing failed
>   */
>
>  #ifndef __DOXYGEN__
>
>  template<>
> -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> +std::optional<bool> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
> -	if (value_ == "true") {
> -		setOk(ok, true);
> +	if (value_ == "true")
>  		return true;
> -	} else if (value_ == "false") {
> -		setOk(ok, true);
> +	else if (value_ == "false")
>  		return false;
> -	}
>
> -	return defaultValue;
> +	return {};
>  }
>
>  template<>
> -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> +std::optional<int16_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int16_t>::min() ||
>  	    value > std::numeric_limits<int16_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> +std::optional<uint16_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint16_t>::min() ||
>  	    value > std::numeric_limits<uint16_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> +std::optional<int32_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int32_t>::min() ||
>  	    value > std::numeric_limits<int32_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> +std::optional<uint32_t> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint32_t>::min() ||
>  	    value > std::numeric_limits<uint32_t>::max())
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -double YamlObject::get(const double &defaultValue, bool *ok) const
> +std::optional<double> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
>  	if (value_ == "")
> -		return defaultValue;
> +		return {};
>
>  	char *end;
>
> @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
>  	double value = std::strtod(value_.c_str(), &end);
>
>  	if ('\0' != *end || errno == ERANGE)
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value;
>  }
>
>  template<>
> -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> +std::optional<std::string> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::Value)
> -		return defaultValue;
> +		return {};
>
> -	setOk(ok, true);
>  	return value_;
>  }
>
>  template<>
> -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> +std::optional<Size> YamlObject::get() const
>  {
> -	setOk(ok, false);
> -
>  	if (type_ != Type::List)
> -		return defaultValue;
> +		return {};
>
>  	if (list_.size() != 2)
> -		return defaultValue;
> +		return {};
>
> -	/*
> -	 * Add a local variable to validate each dimension in case
> -	 * that ok == nullptr.
> -	 */
> -	bool valid;
> -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> -	if (!valid)
> -		return defaultValue;
> +	auto width = list_[0]->get<uint32_t>();
> +	if (!width)
> +		return {};
>
> -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> -	if (!valid)
> -		return defaultValue;
> +	auto height = list_[1]->get<uint32_t>();
> +	if (!height)
> +		return {};
>
> -	setOk(ok, true);
> -	return Size(width, height);
> +	return Size(*width, *height);
>  }
>
>  #endif /* __DOXYGEN__ */
> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> index 38f848232fa6..ebb654f2ef9c 100644
> --- a/test/yaml-parser.cpp
> +++ b/test/yaml-parser.cpp
> @@ -148,7 +148,6 @@ protected:
>  		}
>
>  		/* Test string object */
> -		bool ok;
>  		auto &strObj = (*root)["string"];
>
>  		if (strObj.isDictionary()) {
> @@ -161,27 +160,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> +		if (strObj.get<string>().value_or("") != "libcamera") {
>  			cerr << "String object parse as wrong content" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (strObj.get<int32_t>()) {
>  			cerr << "String object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (strObj.get<uint32_t>()) {
>  			cerr << "String object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (strObj.get<double>()) {
>  			cerr << "String object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (strObj.get<Size>()) {
>  			cerr << "String object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -199,27 +198,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
>  			cerr << "Integer object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> +		if (int32Obj.get<string>().value_or("") != "-100") {
>  			cerr << "Integer object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
>  			cerr << "Integer object fail to parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (int32Obj.get<uint32_t>()) {
>  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (int32Obj.get<Size>()) {
>  			cerr << "Integer object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -237,27 +236,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
>  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> +		if (uint32Obj.get<string>().value_or("") != "100") {
>  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
>  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
>  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (uint32Obj.get<Size>()) {
>  			cerr << "Unsigned integer object parsed as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -275,27 +274,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> +		if (doubleObj.get<string>().value_or("") != "3.14159") {
>  			cerr << "Double object fail to parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
>  			cerr << "Double object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (doubleObj.get<int32_t>()) {
>  			cerr << "Double object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (doubleObj.get<uint32_t>()) {
>  			cerr << "Double object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (doubleObj.get<Size>()) {
>  			cerr << "Double object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -313,27 +312,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<string>("", &ok) != "" || ok) {
> +		if (sizeObj.get<string>()) {
>  			cerr << "Size object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (sizeObj.get<double>()) {
>  			cerr << "Size object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (sizeObj.get<int32_t>()) {
>  			cerr << "Size object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (sizeObj.get<uint32_t>()) {
>  			cerr << "Size object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
>  			cerr << "Size object parse as wrong value" << std::endl;
>  			return TestFail;
>  		}
> @@ -351,27 +350,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (listObj.get<string>("", &ok) != "" || ok) {
> +		if (listObj.get<string>()) {
>  			cerr << "List object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (listObj.get<double>()) {
>  			cerr << "List object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (listObj.get<int32_t>()) {
>  			cerr << "List object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (listObj.get<uint32_t>()) {
>  			cerr << "List object parse as unsigne integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (listObj.get<Size>()) {
>  			cerr << "String list object parse as Size" << std::endl;
>  			return TestFail;
>  		}
> @@ -424,27 +423,27 @@ protected:
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<string>("", &ok) != "" || ok) {
> +		if (dictObj.get<string>()) {
>  			cerr << "Dictionary object parse as string" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> +		if (dictObj.get<double>()) {
>  			cerr << "Dictionary object parse as double" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> +		if (dictObj.get<int32_t>()) {
>  			cerr << "Dictionary object parse as integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> +		if (dictObj.get<uint32_t>()) {
>  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
>  			return TestFail;
>  		}
>
> -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> +		if (dictObj.get<Size>()) {
>  			cerr << "Dictionary object parse as Size" << std::endl;
>  			return TestFail;
>  		}

Hi Christian,

On Tue, Aug 02, 2022 at 10:47:52PM +0200, Christian Rauch via libcamera-devel wrote:
> Dear Laurent,
> 
> The "return {};" in this patch cause "maybe-uninitialized" warnings in
> my CI pipeline:

Thank you for the report. We have CI pipelines too, but clearly, there
are way more compiler/platform combinations out there that we test at
the moment :-)

Could you report the compiler, compiler version and platform, for my
information ?

> ../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp>
> libcamera::YamlObject::get() const [with T = short unsigned int;
> std::enable_if_t<(((((((is_same_v<bool, T> || is_same_v<double, T>) ||
> is_same_v<short int, T>) || is_same_v<short unsigned int, T>) ||
> is_same_v<int, T>) || is_same_v<unsigned int, T>) ||
> is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>,
> std::allocator<char> >, T>) || is_same_v<libcamera::Size, T>)>*
> <anonymous> = 0]’:
> ../src/libcamera/yaml_parser.cpp:184:11: error: ‘<anonymous>’ may be
> used uninitialized in this function [-Werror=maybe-uninitialized]
>   184 |   return {};
>       |           ^
> 
> ... and a couple more in "yaml_parser.cpp".
> 
> Since your intention is here to notify the user of invalid values,
> wouldn't it be better to "return nullptr;" instead?

The would be std::nullopt as the function returns a std::optional<>, but
yes, that would make sense. I was expecting {} to do the same.

As I can't test if this would fix the compilation warning given that I
can't reproduce them, would you be able to test that change with your CI
pipeline ? If std::nullopt fixes the errors, I can send a patch, or you
can send one directly if you prefer.

> Am 28.07.22 um 00:21 schrieb Laurent Pinchart via libcamera-devel:
> > The YamlObject::get() function takes a default value and an optional
> > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > error handling in callers.
> >
> > A better API is possible by dropping the default value and ok flag and
> > returning an std::optional. Not only does it simplify the calls, it also
> > lets callers handle errors through the standard std::optional class
> > instead of the current ad-hoc mechanism.
> >
> > Provide a get() wrapper around std::optional::value_or() to further
> > simplify callers that don't need any specific error handling.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/yaml_parser.h |   9 +-
> >  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> >  test/yaml-parser.cpp                     |  71 ++++++------
> >  3 files changed, 96 insertions(+), 122 deletions(-)
> >
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 064cf44381d7..61f2223223a7 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <iterator>
> >  #include <map>
> > +#include <optional>
> >  #include <string>
> >  #include <vector>
> >
> > @@ -165,7 +166,13 @@ public:
> >  #else
> >  	template<typename T>
> >  #endif
> > -	T get(const T &defaultValue, bool *ok = nullptr) const;
> > +	std::optional<T> get() const;
> > +
> > +	template<typename T>
> > +	T get(const T &defaultValue) const
> > +	{
> > +		return get<T>().value_or(defaultValue);
> > +	}
> >
> >  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> >  	ListAdapter asList() const { return ListAdapter{ list_ }; }
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 5c45e44e49c3..4299f5abd38a 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -31,12 +31,6 @@ namespace {
> >  /* Empty static YamlObject as a safe result for invalid operations */
> >  static const YamlObject empty;
> >
> > -void setOk(bool *ok, bool result)
> > -{
> > -	if (ok)
> > -		*ok = result;
> > -}
> > -
> >  } /* namespace */
> >
> >  /**
> > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> >  }
> >
> >  /**
> > - * \fn template<typename T> YamlObject::get<T>(
> > - *	const T &defaultValue, bool *ok) const
> > + * \fn template<typename T> YamlObject::get<T>() const
> > + * \brief Parse the YamlObject as a \a T value
> > + *
> > + * This function parses the value of the YamlObject as a \a T object, and
> > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > + * store a \a T value), std::nullopt is returned.
> > + *
> > + * \return The YamlObject value, or std::nullopt if parsing failed
> > + */
> > +
> > +/**
> > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> >   * \brief Parse the YamlObject as a \a T value
> >   * \param[in] defaultValue The default value when failing to parse
> > - * \param[out] ok The result of whether the parse succeeded
> >   *
> >   * This function parses the value of the YamlObject as a \a T object, and
> >   * returns the value. If parsing fails (usually because the YamlObject doesn't
> > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > + * store a \a T value), the \a defaultValue is returned.
> >   *
> > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > - * need to know if parsing succeeded.
> > - *
> > - * \return Value as a bool type
> > + * \return The YamlObject value, or \a defaultValue if parsing failed
> >   */
> >
> >  #ifndef __DOXYGEN__
> >
> >  template<>
> > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > +std::optional<bool> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> > -	if (value_ == "true") {
> > -		setOk(ok, true);
> > +	if (value_ == "true")
> >  		return true;
> > -	} else if (value_ == "false") {
> > -		setOk(ok, true);
> > +	else if (value_ == "false")
> >  		return false;
> > -	}
> >
> > -	return defaultValue;
> > +	return {};
> >  }
> >
> >  template<>
> > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > +std::optional<int16_t> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (value_ == "")
> > -		return defaultValue;
> > +		return {};
> >
> >  	char *end;
> >
> > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> >  	if ('\0' != *end || errno == ERANGE ||
> >  	    value < std::numeric_limits<int16_t>::min() ||
> >  	    value > std::numeric_limits<int16_t>::max())
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value;
> >  }
> >
> >  template<>
> > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > +std::optional<uint16_t> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (value_ == "")
> > -		return defaultValue;
> > +		return {};
> >
> >  	/*
> >  	 * libyaml parses all scalar values as strings. When a string has
> > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >  	 */
> >  	std::size_t found = value_.find_first_not_of(" \t");
> >  	if (found != std::string::npos && value_[found] == '-')
> > -		return defaultValue;
> > +		return {};
> >
> >  	char *end;
> >
> > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >  	if ('\0' != *end || errno == ERANGE ||
> >  	    value < std::numeric_limits<uint16_t>::min() ||
> >  	    value > std::numeric_limits<uint16_t>::max())
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value;
> >  }
> >
> >  template<>
> > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > +std::optional<int32_t> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (value_ == "")
> > -		return defaultValue;
> > +		return {};
> >
> >  	char *end;
> >
> > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> >  	if ('\0' != *end || errno == ERANGE ||
> >  	    value < std::numeric_limits<int32_t>::min() ||
> >  	    value > std::numeric_limits<int32_t>::max())
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value;
> >  }
> >
> >  template<>
> > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > +std::optional<uint32_t> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (value_ == "")
> > -		return defaultValue;
> > +		return {};
> >
> >  	/*
> >  	 * libyaml parses all scalar values as strings. When a string has
> > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >  	 */
> >  	std::size_t found = value_.find_first_not_of(" \t");
> >  	if (found != std::string::npos && value_[found] == '-')
> > -		return defaultValue;
> > +		return {};
> >
> >  	char *end;
> >
> > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >  	if ('\0' != *end || errno == ERANGE ||
> >  	    value < std::numeric_limits<uint32_t>::min() ||
> >  	    value > std::numeric_limits<uint32_t>::max())
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value;
> >  }
> >
> >  template<>
> > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > +std::optional<double> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (value_ == "")
> > -		return defaultValue;
> > +		return {};
> >
> >  	char *end;
> >
> > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> >  	double value = std::strtod(value_.c_str(), &end);
> >
> >  	if ('\0' != *end || errno == ERANGE)
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value;
> >  }
> >
> >  template<>
> > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > +std::optional<std::string> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::Value)
> > -		return defaultValue;
> > +		return {};
> >
> > -	setOk(ok, true);
> >  	return value_;
> >  }
> >
> >  template<>
> > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > +std::optional<Size> YamlObject::get() const
> >  {
> > -	setOk(ok, false);
> > -
> >  	if (type_ != Type::List)
> > -		return defaultValue;
> > +		return {};
> >
> >  	if (list_.size() != 2)
> > -		return defaultValue;
> > +		return {};
> >
> > -	/*
> > -	 * Add a local variable to validate each dimension in case
> > -	 * that ok == nullptr.
> > -	 */
> > -	bool valid;
> > -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > -	if (!valid)
> > -		return defaultValue;
> > +	auto width = list_[0]->get<uint32_t>();
> > +	if (!width)
> > +		return {};
> >
> > -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > -	if (!valid)
> > -		return defaultValue;
> > +	auto height = list_[1]->get<uint32_t>();
> > +	if (!height)
> > +		return {};
> >
> > -	setOk(ok, true);
> > -	return Size(width, height);
> > +	return Size(*width, *height);
> >  }
> >
> >  #endif /* __DOXYGEN__ */
> > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > index 38f848232fa6..ebb654f2ef9c 100644
> > --- a/test/yaml-parser.cpp
> > +++ b/test/yaml-parser.cpp
> > @@ -148,7 +148,6 @@ protected:
> >  		}
> >
> >  		/* Test string object */
> > -		bool ok;
> >  		auto &strObj = (*root)["string"];
> >
> >  		if (strObj.isDictionary()) {
> > @@ -161,27 +160,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > +		if (strObj.get<string>().value_or("") != "libcamera") {
> >  			cerr << "String object parse as wrong content" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +		if (strObj.get<int32_t>()) {
> >  			cerr << "String object parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (strObj.get<uint32_t>()) {
> >  			cerr << "String object parse as unsigned integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +		if (strObj.get<double>()) {
> >  			cerr << "String object parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (strObj.get<Size>()) {
> >  			cerr << "String object parse as Size" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -199,27 +198,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
> >  			cerr << "Integer object parse as wrong value" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > +		if (int32Obj.get<string>().value_or("") != "-100") {
> >  			cerr << "Integer object fail to parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> >  			cerr << "Integer object fail to parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (int32Obj.get<uint32_t>()) {
> >  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (int32Obj.get<Size>()) {
> >  			cerr << "Integer object parse as Size" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -237,27 +236,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> >  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > +		if (uint32Obj.get<string>().value_or("") != "100") {
> >  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> >  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> >  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (uint32Obj.get<Size>()) {
> >  			cerr << "Unsigned integer object parsed as Size" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -275,27 +274,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > +		if (doubleObj.get<string>().value_or("") != "3.14159") {
> >  			cerr << "Double object fail to parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> >  			cerr << "Double object parse as wrong value" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +		if (doubleObj.get<int32_t>()) {
> >  			cerr << "Double object parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (doubleObj.get<uint32_t>()) {
> >  			cerr << "Double object parse as unsigned integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (doubleObj.get<Size>()) {
> >  			cerr << "Double object parse as Size" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -313,27 +312,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (sizeObj.get<string>("", &ok) != "" || ok) {
> > +		if (sizeObj.get<string>()) {
> >  			cerr << "Size object parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +		if (sizeObj.get<double>()) {
> >  			cerr << "Size object parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +		if (sizeObj.get<int32_t>()) {
> >  			cerr << "Size object parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (sizeObj.get<uint32_t>()) {
> >  			cerr << "Size object parse as unsigned integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> >  			cerr << "Size object parse as wrong value" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -351,27 +350,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (listObj.get<string>("", &ok) != "" || ok) {
> > +		if (listObj.get<string>()) {
> >  			cerr << "List object parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +		if (listObj.get<double>()) {
> >  			cerr << "List object parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +		if (listObj.get<int32_t>()) {
> >  			cerr << "List object parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (listObj.get<uint32_t>()) {
> >  			cerr << "List object parse as unsigne integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (listObj.get<Size>()) {
> >  			cerr << "String list object parse as Size" << std::endl;
> >  			return TestFail;
> >  		}
> > @@ -424,27 +423,27 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		if (dictObj.get<string>("", &ok) != "" || ok) {
> > +		if (dictObj.get<string>()) {
> >  			cerr << "Dictionary object parse as string" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > +		if (dictObj.get<double>()) {
> >  			cerr << "Dictionary object parse as double" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > +		if (dictObj.get<int32_t>()) {
> >  			cerr << "Dictionary object parse as integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > +		if (dictObj.get<uint32_t>()) {
> >  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
> >  			return TestFail;
> >  		}
> >
> > -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > +		if (dictObj.get<Size>()) {
> >  			cerr << "Dictionary object parse as Size" << std::endl;
> >  			return TestFail;
> >  		}

Hi Laurent,

Am 03.08.22 um 00:01 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Tue, Aug 02, 2022 at 10:47:52PM +0200, Christian Rauch via libcamera-devel wrote:
>> Dear Laurent,
>>
>> The "return {};" in this patch cause "maybe-uninitialized" warnings in
>> my CI pipeline:
>
> Thank you for the report. We have CI pipelines too, but clearly, there
> are way more compiler/platform combinations out there that we test at
> the moment :-)
>
> Could you report the compiler, compiler version and platform, for my
> information ?

It's an arm64 corss compile workflow with gcc 9 [1]. See the full log at
[2].

[1]
https://github.com/christianrauch/libcamera-ci/blob/ci/.github/workflows/main_cross.yml
[2]
https://github.com/christianrauch/libcamera-ci/runs/7639256594?check_suite_focus=true

>
>> ../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp>
>> libcamera::YamlObject::get() const [with T = short unsigned int;
>> std::enable_if_t<(((((((is_same_v<bool, T> || is_same_v<double, T>) ||
>> is_same_v<short int, T>) || is_same_v<short unsigned int, T>) ||
>> is_same_v<int, T>) || is_same_v<unsigned int, T>) ||
>> is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>,
>> std::allocator<char> >, T>) || is_same_v<libcamera::Size, T>)>*
>> <anonymous> = 0]’:
>> ../src/libcamera/yaml_parser.cpp:184:11: error: ‘<anonymous>’ may be
>> used uninitialized in this function [-Werror=maybe-uninitialized]
>>   184 |   return {};
>>       |           ^
>>
>> ... and a couple more in "yaml_parser.cpp".
>>
>> Since your intention is here to notify the user of invalid values,
>> wouldn't it be better to "return nullptr;" instead?
>
> The would be std::nullopt as the function returns a std::optional<>, but
> yes, that would make sense. I was expecting {} to do the same.
>
> As I can't test if this would fix the compilation warning given that I
> can't reproduce them, would you be able to test that change with your CI
> pipeline ? If std::nullopt fixes the errors, I can send a patch, or you
> can send one directly if you prefer.
>
>> Am 28.07.22 um 00:21 schrieb Laurent Pinchart via libcamera-devel:
>>> The YamlObject::get() function takes a default value and an optional
>>> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
>>> error handling in callers.
>>>
>>> A better API is possible by dropping the default value and ok flag and
>>> returning an std::optional. Not only does it simplify the calls, it also
>>> lets callers handle errors through the standard std::optional class
>>> instead of the current ad-hoc mechanism.
>>>
>>> Provide a get() wrapper around std::optional::value_or() to further
>>> simplify callers that don't need any specific error handling.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>>> Tested-by: Naushir Patuck <naush@raspberrypi.com>
>>> ---
>>>  include/libcamera/internal/yaml_parser.h |   9 +-
>>>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
>>>  test/yaml-parser.cpp                     |  71 ++++++------
>>>  3 files changed, 96 insertions(+), 122 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
>>> index 064cf44381d7..61f2223223a7 100644
>>> --- a/include/libcamera/internal/yaml_parser.h
>>> +++ b/include/libcamera/internal/yaml_parser.h
>>> @@ -9,6 +9,7 @@
>>>
>>>  #include <iterator>
>>>  #include <map>
>>> +#include <optional>
>>>  #include <string>
>>>  #include <vector>
>>>
>>> @@ -165,7 +166,13 @@ public:
>>>  #else
>>>  	template<typename T>
>>>  #endif
>>> -	T get(const T &defaultValue, bool *ok = nullptr) const;
>>> +	std::optional<T> get() const;
>>> +
>>> +	template<typename T>
>>> +	T get(const T &defaultValue) const
>>> +	{
>>> +		return get<T>().value_or(defaultValue);
>>> +	}
>>>
>>>  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
>>>  	ListAdapter asList() const { return ListAdapter{ list_ }; }
>>> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
>>> index 5c45e44e49c3..4299f5abd38a 100644
>>> --- a/src/libcamera/yaml_parser.cpp
>>> +++ b/src/libcamera/yaml_parser.cpp
>>> @@ -31,12 +31,6 @@ namespace {
>>>  /* Empty static YamlObject as a safe result for invalid operations */
>>>  static const YamlObject empty;
>>>
>>> -void setOk(bool *ok, bool result)
>>> -{
>>> -	if (ok)
>>> -		*ok = result;
>>> -}
>>> -
>>>  } /* namespace */
>>>
>>>  /**
>>> @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
>>>  }
>>>
>>>  /**
>>> - * \fn template<typename T> YamlObject::get<T>(
>>> - *	const T &defaultValue, bool *ok) const
>>> + * \fn template<typename T> YamlObject::get<T>() const
>>> + * \brief Parse the YamlObject as a \a T value
>>> + *
>>> + * This function parses the value of the YamlObject as a \a T object, and
>>> + * returns the value. If parsing fails (usually because the YamlObject doesn't
>>> + * store a \a T value), std::nullopt is returned.
>>> + *
>>> + * \return The YamlObject value, or std::nullopt if parsing failed
>>> + */
>>> +
>>> +/**
>>> + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
>>>   * \brief Parse the YamlObject as a \a T value
>>>   * \param[in] defaultValue The default value when failing to parse
>>> - * \param[out] ok The result of whether the parse succeeded
>>>   *
>>>   * This function parses the value of the YamlObject as a \a T object, and
>>>   * returns the value. If parsing fails (usually because the YamlObject doesn't
>>> - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
>>> - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
>>> + * store a \a T value), the \a defaultValue is returned.
>>>   *
>>> - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
>>> - * need to know if parsing succeeded.
>>> - *
>>> - * \return Value as a bool type
>>> + * \return The YamlObject value, or \a defaultValue if parsing failed
>>>   */
>>>
>>>  #ifndef __DOXYGEN__
>>>
>>>  template<>
>>> -bool YamlObject::get(const bool &defaultValue, bool *ok) const
>>> +std::optional<bool> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	if (value_ == "true") {
>>> -		setOk(ok, true);
>>> +	if (value_ == "true")
>>>  		return true;
>>> -	} else if (value_ == "false") {
>>> -		setOk(ok, true);
>>> +	else if (value_ == "false")
>>>  		return false;
>>> -	}
>>>
>>> -	return defaultValue;
>>> +	return {};
>>>  }
>>>
>>>  template<>
>>> -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>>> +std::optional<int16_t> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (value_ == "")
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	char *end;
>>>
>>> @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>>>  	if ('\0' != *end || errno == ERANGE ||
>>>  	    value < std::numeric_limits<int16_t>::min() ||
>>>  	    value > std::numeric_limits<int16_t>::max())
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value;
>>>  }
>>>
>>>  template<>
>>> -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>> +std::optional<uint16_t> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (value_ == "")
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	/*
>>>  	 * libyaml parses all scalar values as strings. When a string has
>>> @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>>  	 */
>>>  	std::size_t found = value_.find_first_not_of(" \t");
>>>  	if (found != std::string::npos && value_[found] == '-')
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	char *end;
>>>
>>> @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>>  	if ('\0' != *end || errno == ERANGE ||
>>>  	    value < std::numeric_limits<uint16_t>::min() ||
>>>  	    value > std::numeric_limits<uint16_t>::max())
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value;
>>>  }
>>>
>>>  template<>
>>> -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>>> +std::optional<int32_t> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (value_ == "")
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	char *end;
>>>
>>> @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>>>  	if ('\0' != *end || errno == ERANGE ||
>>>  	    value < std::numeric_limits<int32_t>::min() ||
>>>  	    value > std::numeric_limits<int32_t>::max())
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value;
>>>  }
>>>
>>>  template<>
>>> -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>> +std::optional<uint32_t> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (value_ == "")
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	/*
>>>  	 * libyaml parses all scalar values as strings. When a string has
>>> @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>>  	 */
>>>  	std::size_t found = value_.find_first_not_of(" \t");
>>>  	if (found != std::string::npos && value_[found] == '-')
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	char *end;
>>>
>>> @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>>  	if ('\0' != *end || errno == ERANGE ||
>>>  	    value < std::numeric_limits<uint32_t>::min() ||
>>>  	    value > std::numeric_limits<uint32_t>::max())
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value;
>>>  }
>>>
>>>  template<>
>>> -double YamlObject::get(const double &defaultValue, bool *ok) const
>>> +std::optional<double> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (value_ == "")
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	char *end;
>>>
>>> @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
>>>  	double value = std::strtod(value_.c_str(), &end);
>>>
>>>  	if ('\0' != *end || errno == ERANGE)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value;
>>>  }
>>>
>>>  template<>
>>> -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
>>> +std::optional<std::string> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::Value)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>>  	return value_;
>>>  }
>>>
>>>  template<>
>>> -Size YamlObject::get(const Size &defaultValue, bool *ok) const
>>> +std::optional<Size> YamlObject::get() const
>>>  {
>>> -	setOk(ok, false);
>>> -
>>>  	if (type_ != Type::List)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>>  	if (list_.size() != 2)
>>> -		return defaultValue;
>>> +		return {};
>>>
>>> -	/*
>>> -	 * Add a local variable to validate each dimension in case
>>> -	 * that ok == nullptr.
>>> -	 */
>>> -	bool valid;
>>> -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
>>> -	if (!valid)
>>> -		return defaultValue;
>>> +	auto width = list_[0]->get<uint32_t>();
>>> +	if (!width)
>>> +		return {};
>>>
>>> -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
>>> -	if (!valid)
>>> -		return defaultValue;
>>> +	auto height = list_[1]->get<uint32_t>();
>>> +	if (!height)
>>> +		return {};
>>>
>>> -	setOk(ok, true);
>>> -	return Size(width, height);
>>> +	return Size(*width, *height);
>>>  }
>>>
>>>  #endif /* __DOXYGEN__ */
>>> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
>>> index 38f848232fa6..ebb654f2ef9c 100644
>>> --- a/test/yaml-parser.cpp
>>> +++ b/test/yaml-parser.cpp
>>> @@ -148,7 +148,6 @@ protected:
>>>  		}
>>>
>>>  		/* Test string object */
>>> -		bool ok;
>>>  		auto &strObj = (*root)["string"];
>>>
>>>  		if (strObj.isDictionary()) {
>>> @@ -161,27 +160,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
>>> +		if (strObj.get<string>().value_or("") != "libcamera") {
>>>  			cerr << "String object parse as wrong content" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>> +		if (strObj.get<int32_t>()) {
>>>  			cerr << "String object parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (strObj.get<uint32_t>()) {
>>>  			cerr << "String object parse as unsigned integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>> +		if (strObj.get<double>()) {
>>>  			cerr << "String object parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (strObj.get<Size>()) {
>>>  			cerr << "String object parse as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -199,27 +198,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
>>> +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
>>>  			cerr << "Integer object parse as wrong value" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
>>> +		if (int32Obj.get<string>().value_or("") != "-100") {
>>>  			cerr << "Integer object fail to parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
>>> +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
>>>  			cerr << "Integer object fail to parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (int32Obj.get<uint32_t>()) {
>>>  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (int32Obj.get<Size>()) {
>>>  			cerr << "Integer object parse as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -237,27 +236,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
>>> +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
>>>  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
>>> +		if (uint32Obj.get<string>().value_or("") != "100") {
>>>  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
>>> +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
>>>  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
>>> +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
>>>  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (uint32Obj.get<Size>()) {
>>>  			cerr << "Unsigned integer object parsed as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -275,27 +274,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
>>> +		if (doubleObj.get<string>().value_or("") != "3.14159") {
>>>  			cerr << "Double object fail to parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
>>> +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
>>>  			cerr << "Double object parse as wrong value" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>> +		if (doubleObj.get<int32_t>()) {
>>>  			cerr << "Double object parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (doubleObj.get<uint32_t>()) {
>>>  			cerr << "Double object parse as unsigned integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (doubleObj.get<Size>()) {
>>>  			cerr << "Double object parse as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -313,27 +312,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (sizeObj.get<string>("", &ok) != "" || ok) {
>>> +		if (sizeObj.get<string>()) {
>>>  			cerr << "Size object parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>> +		if (sizeObj.get<double>()) {
>>>  			cerr << "Size object parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>> +		if (sizeObj.get<int32_t>()) {
>>>  			cerr << "Size object parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (sizeObj.get<uint32_t>()) {
>>>  			cerr << "Size object parse as unsigned integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
>>> +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
>>>  			cerr << "Size object parse as wrong value" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -351,27 +350,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (listObj.get<string>("", &ok) != "" || ok) {
>>> +		if (listObj.get<string>()) {
>>>  			cerr << "List object parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>> +		if (listObj.get<double>()) {
>>>  			cerr << "List object parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>> +		if (listObj.get<int32_t>()) {
>>>  			cerr << "List object parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (listObj.get<uint32_t>()) {
>>>  			cerr << "List object parse as unsigne integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (listObj.get<Size>()) {
>>>  			cerr << "String list object parse as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>>> @@ -424,27 +423,27 @@ protected:
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (dictObj.get<string>("", &ok) != "" || ok) {
>>> +		if (dictObj.get<string>()) {
>>>  			cerr << "Dictionary object parse as string" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>> +		if (dictObj.get<double>()) {
>>>  			cerr << "Dictionary object parse as double" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>> +		if (dictObj.get<int32_t>()) {
>>>  			cerr << "Dictionary object parse as integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>> +		if (dictObj.get<uint32_t>()) {
>>>  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
>>>  			return TestFail;
>>>  		}
>>>
>>> -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>> +		if (dictObj.get<Size>()) {
>>>  			cerr << "Dictionary object parse as Size" << std::endl;
>>>  			return TestFail;
>>>  		}
>

Hi Christian,

On Wed, Aug 03, 2022 at 09:10:19PM +0200, Christian Rauch via libcamera-devel wrote:
> Am 03.08.22 um 00:01 schrieb Laurent Pinchart:
> > On Tue, Aug 02, 2022 at 10:47:52PM +0200, Christian Rauch via libcamera-devel wrote:
> >> Dear Laurent,
> >>
> >> The "return {};" in this patch cause "maybe-uninitialized" warnings in
> >> my CI pipeline:
> >
> > Thank you for the report. We have CI pipelines too, but clearly, there
> > are way more compiler/platform combinations out there that we test at
> > the moment :-)
> >
> > Could you report the compiler, compiler version and platform, for my
> > information ?
> 
> It's an arm64 corss compile workflow with gcc 9 [1]. See the full log at
> [2].
> 
> [1] https://github.com/christianrauch/libcamera-ci/blob/ci/.github/workflows/main_cross.yml
> [2] https://github.com/christianrauch/libcamera-ci/runs/7639256594?check_suite_focus=true

Could you test the following patch to see if it fixes the problem ? If
so I'll send it with a proper commit message.

diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
index c96e99e1317c..9162e2250ed4 100644
--- a/src/libcamera/yaml_parser.cpp
+++ b/src/libcamera/yaml_parser.cpp
@@ -121,24 +121,24 @@ template<>
 std::optional<bool> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "true")
 		return true;
 	else if (value_ == "false")
 		return false;

-	return {};
+	return std::nullopt;
 }

 template<>
 std::optional<int16_t> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "")
-		return {};
+		return std::nullopt;

 	char *end;

@@ -148,7 +148,7 @@ std::optional<int16_t> YamlObject::get() const
 	if ('\0' != *end || errno == ERANGE ||
 	    value < std::numeric_limits<int16_t>::min() ||
 	    value > std::numeric_limits<int16_t>::max())
-		return {};
+		return std::nullopt;

 	return value;
 }
@@ -157,10 +157,10 @@ template<>
 std::optional<uint16_t> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "")
-		return {};
+		return std::nullopt;

 	/*
 	 * libyaml parses all scalar values as strings. When a string has
@@ -171,7 +171,7 @@ std::optional<uint16_t> YamlObject::get() const
 	 */
 	std::size_t found = value_.find_first_not_of(" \t");
 	if (found != std::string::npos && value_[found] == '-')
-		return {};
+		return std::nullopt;

 	char *end;

@@ -181,7 +181,7 @@ std::optional<uint16_t> YamlObject::get() const
 	if ('\0' != *end || errno == ERANGE ||
 	    value < std::numeric_limits<uint16_t>::min() ||
 	    value > std::numeric_limits<uint16_t>::max())
-		return {};
+		return std::nullopt;

 	return value;
 }
@@ -190,10 +190,10 @@ template<>
 std::optional<int32_t> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "")
-		return {};
+		return std::nullopt;

 	char *end;

@@ -203,7 +203,7 @@ std::optional<int32_t> YamlObject::get() const
 	if ('\0' != *end || errno == ERANGE ||
 	    value < std::numeric_limits<int32_t>::min() ||
 	    value > std::numeric_limits<int32_t>::max())
-		return {};
+		return std::nullopt;

 	return value;
 }
@@ -212,10 +212,10 @@ template<>
 std::optional<uint32_t> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "")
-		return {};
+		return std::nullopt;

 	/*
 	 * libyaml parses all scalar values as strings. When a string has
@@ -226,7 +226,7 @@ std::optional<uint32_t> YamlObject::get() const
 	 */
 	std::size_t found = value_.find_first_not_of(" \t");
 	if (found != std::string::npos && value_[found] == '-')
-		return {};
+		return std::nullopt;

 	char *end;

@@ -236,7 +236,7 @@ std::optional<uint32_t> YamlObject::get() const
 	if ('\0' != *end || errno == ERANGE ||
 	    value < std::numeric_limits<uint32_t>::min() ||
 	    value > std::numeric_limits<uint32_t>::max())
-		return {};
+		return std::nullopt;

 	return value;
 }
@@ -245,10 +245,10 @@ template<>
 std::optional<double> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	if (value_ == "")
-		return {};
+		return std::nullopt;

 	char *end;

@@ -256,7 +256,7 @@ std::optional<double> YamlObject::get() const
 	double value = std::strtod(value_.c_str(), &end);

 	if ('\0' != *end || errno == ERANGE)
-		return {};
+		return std::nullopt;

 	return value;
 }
@@ -265,7 +265,7 @@ template<>
 std::optional<std::string> YamlObject::get() const
 {
 	if (type_ != Type::Value)
-		return {};
+		return std::nullopt;

 	return value_;
 }
@@ -274,18 +274,18 @@ template<>
 std::optional<Size> YamlObject::get() const
 {
 	if (type_ != Type::List)
-		return {};
+		return std::nullopt;

 	if (list_.size() != 2)
-		return {};
+		return std::nullopt;

 	auto width = list_[0].value->get<uint32_t>();
 	if (!width)
-		return {};
+		return std::nullopt;

 	auto height = list_[1].value->get<uint32_t>();
 	if (!height)
-		return {};
+		return std::nullopt;

 	return Size(*width, *height);
 }


> >> ../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp>
> >> libcamera::YamlObject::get() const [with T = short unsigned int;
> >> std::enable_if_t<(((((((is_same_v<bool, T> || is_same_v<double, T>) ||
> >> is_same_v<short int, T>) || is_same_v<short unsigned int, T>) ||
> >> is_same_v<int, T>) || is_same_v<unsigned int, T>) ||
> >> is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>,
> >> std::allocator<char> >, T>) || is_same_v<libcamera::Size, T>)>*
> >> <anonymous> = 0]’:
> >> ../src/libcamera/yaml_parser.cpp:184:11: error: ‘<anonymous>’ may be
> >> used uninitialized in this function [-Werror=maybe-uninitialized]
> >>   184 |   return {};
> >>       |           ^
> >>
> >> ... and a couple more in "yaml_parser.cpp".
> >>
> >> Since your intention is here to notify the user of invalid values,
> >> wouldn't it be better to "return nullptr;" instead?
> >
> > The would be std::nullopt as the function returns a std::optional<>, but
> > yes, that would make sense. I was expecting {} to do the same.
> >
> > As I can't test if this would fix the compilation warning given that I
> > can't reproduce them, would you be able to test that change with your CI
> > pipeline ? If std::nullopt fixes the errors, I can send a patch, or you
> > can send one directly if you prefer.
> >
> >> Am 28.07.22 um 00:21 schrieb Laurent Pinchart via libcamera-devel:
> >>> The YamlObject::get() function takes a default value and an optional
> >>> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> >>> error handling in callers.
> >>>
> >>> A better API is possible by dropping the default value and ok flag and
> >>> returning an std::optional. Not only does it simplify the calls, it also
> >>> lets callers handle errors through the standard std::optional class
> >>> instead of the current ad-hoc mechanism.
> >>>
> >>> Provide a get() wrapper around std::optional::value_or() to further
> >>> simplify callers that don't need any specific error handling.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> >>> Tested-by: Naushir Patuck <naush@raspberrypi.com>
> >>> ---
> >>>  include/libcamera/internal/yaml_parser.h |   9 +-
> >>>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
> >>>  test/yaml-parser.cpp                     |  71 ++++++------
> >>>  3 files changed, 96 insertions(+), 122 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> >>> index 064cf44381d7..61f2223223a7 100644
> >>> --- a/include/libcamera/internal/yaml_parser.h
> >>> +++ b/include/libcamera/internal/yaml_parser.h
> >>> @@ -9,6 +9,7 @@
> >>>
> >>>  #include <iterator>
> >>>  #include <map>
> >>> +#include <optional>
> >>>  #include <string>
> >>>  #include <vector>
> >>>
> >>> @@ -165,7 +166,13 @@ public:
> >>>  #else
> >>>  	template<typename T>
> >>>  #endif
> >>> -	T get(const T &defaultValue, bool *ok = nullptr) const;
> >>> +	std::optional<T> get() const;
> >>> +
> >>> +	template<typename T>
> >>> +	T get(const T &defaultValue) const
> >>> +	{
> >>> +		return get<T>().value_or(defaultValue);
> >>> +	}
> >>>
> >>>  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> >>>  	ListAdapter asList() const { return ListAdapter{ list_ }; }
> >>> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> >>> index 5c45e44e49c3..4299f5abd38a 100644
> >>> --- a/src/libcamera/yaml_parser.cpp
> >>> +++ b/src/libcamera/yaml_parser.cpp
> >>> @@ -31,12 +31,6 @@ namespace {
> >>>  /* Empty static YamlObject as a safe result for invalid operations */
> >>>  static const YamlObject empty;
> >>>
> >>> -void setOk(bool *ok, bool result)
> >>> -{
> >>> -	if (ok)
> >>> -		*ok = result;
> >>> -}
> >>> -
> >>>  } /* namespace */
> >>>
> >>>  /**
> >>> @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> >>>  }
> >>>
> >>>  /**
> >>> - * \fn template<typename T> YamlObject::get<T>(
> >>> - *	const T &defaultValue, bool *ok) const
> >>> + * \fn template<typename T> YamlObject::get<T>() const
> >>> + * \brief Parse the YamlObject as a \a T value
> >>> + *
> >>> + * This function parses the value of the YamlObject as a \a T object, and
> >>> + * returns the value. If parsing fails (usually because the YamlObject doesn't
> >>> + * store a \a T value), std::nullopt is returned.
> >>> + *
> >>> + * \return The YamlObject value, or std::nullopt if parsing failed
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> >>>   * \brief Parse the YamlObject as a \a T value
> >>>   * \param[in] defaultValue The default value when failing to parse
> >>> - * \param[out] ok The result of whether the parse succeeded
> >>>   *
> >>>   * This function parses the value of the YamlObject as a \a T object, and
> >>>   * returns the value. If parsing fails (usually because the YamlObject doesn't
> >>> - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> >>> - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> >>> + * store a \a T value), the \a defaultValue is returned.
> >>>   *
> >>> - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> >>> - * need to know if parsing succeeded.
> >>> - *
> >>> - * \return Value as a bool type
> >>> + * \return The YamlObject value, or \a defaultValue if parsing failed
> >>>   */
> >>>
> >>>  #ifndef __DOXYGEN__
> >>>
> >>>  template<>
> >>> -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> >>> +std::optional<bool> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	if (value_ == "true") {
> >>> -		setOk(ok, true);
> >>> +	if (value_ == "true")
> >>>  		return true;
> >>> -	} else if (value_ == "false") {
> >>> -		setOk(ok, true);
> >>> +	else if (value_ == "false")
> >>>  		return false;
> >>> -	}
> >>>
> >>> -	return defaultValue;
> >>> +	return {};
> >>>  }
> >>>
> >>>  template<>
> >>> -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> >>> +std::optional<int16_t> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (value_ == "")
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	char *end;
> >>>
> >>> @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> >>>  	if ('\0' != *end || errno == ERANGE ||
> >>>  	    value < std::numeric_limits<int16_t>::min() ||
> >>>  	    value > std::numeric_limits<int16_t>::max())
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value;
> >>>  }
> >>>
> >>>  template<>
> >>> -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >>> +std::optional<uint16_t> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (value_ == "")
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	/*
> >>>  	 * libyaml parses all scalar values as strings. When a string has
> >>> @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >>>  	 */
> >>>  	std::size_t found = value_.find_first_not_of(" \t");
> >>>  	if (found != std::string::npos && value_[found] == '-')
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	char *end;
> >>>
> >>> @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> >>>  	if ('\0' != *end || errno == ERANGE ||
> >>>  	    value < std::numeric_limits<uint16_t>::min() ||
> >>>  	    value > std::numeric_limits<uint16_t>::max())
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value;
> >>>  }
> >>>
> >>>  template<>
> >>> -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> >>> +std::optional<int32_t> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (value_ == "")
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	char *end;
> >>>
> >>> @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> >>>  	if ('\0' != *end || errno == ERANGE ||
> >>>  	    value < std::numeric_limits<int32_t>::min() ||
> >>>  	    value > std::numeric_limits<int32_t>::max())
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value;
> >>>  }
> >>>
> >>>  template<>
> >>> -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >>> +std::optional<uint32_t> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (value_ == "")
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	/*
> >>>  	 * libyaml parses all scalar values as strings. When a string has
> >>> @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >>>  	 */
> >>>  	std::size_t found = value_.find_first_not_of(" \t");
> >>>  	if (found != std::string::npos && value_[found] == '-')
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	char *end;
> >>>
> >>> @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >>>  	if ('\0' != *end || errno == ERANGE ||
> >>>  	    value < std::numeric_limits<uint32_t>::min() ||
> >>>  	    value > std::numeric_limits<uint32_t>::max())
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value;
> >>>  }
> >>>
> >>>  template<>
> >>> -double YamlObject::get(const double &defaultValue, bool *ok) const
> >>> +std::optional<double> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (value_ == "")
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	char *end;
> >>>
> >>> @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> >>>  	double value = std::strtod(value_.c_str(), &end);
> >>>
> >>>  	if ('\0' != *end || errno == ERANGE)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value;
> >>>  }
> >>>
> >>>  template<>
> >>> -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> >>> +std::optional<std::string> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::Value)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>>  	return value_;
> >>>  }
> >>>
> >>>  template<>
> >>> -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> >>> +std::optional<Size> YamlObject::get() const
> >>>  {
> >>> -	setOk(ok, false);
> >>> -
> >>>  	if (type_ != Type::List)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>>  	if (list_.size() != 2)
> >>> -		return defaultValue;
> >>> +		return {};
> >>>
> >>> -	/*
> >>> -	 * Add a local variable to validate each dimension in case
> >>> -	 * that ok == nullptr.
> >>> -	 */
> >>> -	bool valid;
> >>> -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> >>> -	if (!valid)
> >>> -		return defaultValue;
> >>> +	auto width = list_[0]->get<uint32_t>();
> >>> +	if (!width)
> >>> +		return {};
> >>>
> >>> -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> >>> -	if (!valid)
> >>> -		return defaultValue;
> >>> +	auto height = list_[1]->get<uint32_t>();
> >>> +	if (!height)
> >>> +		return {};
> >>>
> >>> -	setOk(ok, true);
> >>> -	return Size(width, height);
> >>> +	return Size(*width, *height);
> >>>  }
> >>>
> >>>  #endif /* __DOXYGEN__ */
> >>> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> >>> index 38f848232fa6..ebb654f2ef9c 100644
> >>> --- a/test/yaml-parser.cpp
> >>> +++ b/test/yaml-parser.cpp
> >>> @@ -148,7 +148,6 @@ protected:
> >>>  		}
> >>>
> >>>  		/* Test string object */
> >>> -		bool ok;
> >>>  		auto &strObj = (*root)["string"];
> >>>
> >>>  		if (strObj.isDictionary()) {
> >>> @@ -161,27 +160,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> >>> +		if (strObj.get<string>().value_or("") != "libcamera") {
> >>>  			cerr << "String object parse as wrong content" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> >>> +		if (strObj.get<int32_t>()) {
> >>>  			cerr << "String object parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (strObj.get<uint32_t>()) {
> >>>  			cerr << "String object parse as unsigned integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> >>> +		if (strObj.get<double>()) {
> >>>  			cerr << "String object parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (strObj.get<Size>()) {
> >>>  			cerr << "String object parse as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -199,27 +198,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> >>> +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
> >>>  			cerr << "Integer object parse as wrong value" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> >>> +		if (int32Obj.get<string>().value_or("") != "-100") {
> >>>  			cerr << "Integer object fail to parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> >>> +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> >>>  			cerr << "Integer object fail to parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (int32Obj.get<uint32_t>()) {
> >>>  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (int32Obj.get<Size>()) {
> >>>  			cerr << "Integer object parse as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -237,27 +236,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> >>> +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> >>>  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> >>> +		if (uint32Obj.get<string>().value_or("") != "100") {
> >>>  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> >>> +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> >>>  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> >>> +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> >>>  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (uint32Obj.get<Size>()) {
> >>>  			cerr << "Unsigned integer object parsed as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -275,27 +274,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> >>> +		if (doubleObj.get<string>().value_or("") != "3.14159") {
> >>>  			cerr << "Double object fail to parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> >>> +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> >>>  			cerr << "Double object parse as wrong value" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> >>> +		if (doubleObj.get<int32_t>()) {
> >>>  			cerr << "Double object parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (doubleObj.get<uint32_t>()) {
> >>>  			cerr << "Double object parse as unsigned integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (doubleObj.get<Size>()) {
> >>>  			cerr << "Double object parse as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -313,27 +312,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (sizeObj.get<string>("", &ok) != "" || ok) {
> >>> +		if (sizeObj.get<string>()) {
> >>>  			cerr << "Size object parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> >>> +		if (sizeObj.get<double>()) {
> >>>  			cerr << "Size object parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> >>> +		if (sizeObj.get<int32_t>()) {
> >>>  			cerr << "Size object parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (sizeObj.get<uint32_t>()) {
> >>>  			cerr << "Size object parse as unsigned integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> >>> +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> >>>  			cerr << "Size object parse as wrong value" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -351,27 +350,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (listObj.get<string>("", &ok) != "" || ok) {
> >>> +		if (listObj.get<string>()) {
> >>>  			cerr << "List object parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> >>> +		if (listObj.get<double>()) {
> >>>  			cerr << "List object parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> >>> +		if (listObj.get<int32_t>()) {
> >>>  			cerr << "List object parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (listObj.get<uint32_t>()) {
> >>>  			cerr << "List object parse as unsigne integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (listObj.get<Size>()) {
> >>>  			cerr << "String list object parse as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>> @@ -424,27 +423,27 @@ protected:
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (dictObj.get<string>("", &ok) != "" || ok) {
> >>> +		if (dictObj.get<string>()) {
> >>>  			cerr << "Dictionary object parse as string" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> >>> +		if (dictObj.get<double>()) {
> >>>  			cerr << "Dictionary object parse as double" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> >>> +		if (dictObj.get<int32_t>()) {
> >>>  			cerr << "Dictionary object parse as integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> >>> +		if (dictObj.get<uint32_t>()) {
> >>>  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
> >>>  			return TestFail;
> >>>  		}
> >>>
> >>> -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> >>> +		if (dictObj.get<Size>()) {
> >>>  			cerr << "Dictionary object parse as Size" << std::endl;
> >>>  			return TestFail;
> >>>  		}

Thanks Laurent, the patch seems to work [1].

[1] https://github.com/christianrauch/libcamera-ci/actions/runs/2799422430

Am 04.08.22 um 00:12 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Wed, Aug 03, 2022 at 09:10:19PM +0200, Christian Rauch via libcamera-devel wrote:
>> Am 03.08.22 um 00:01 schrieb Laurent Pinchart:
>>> On Tue, Aug 02, 2022 at 10:47:52PM +0200, Christian Rauch via libcamera-devel wrote:
>>>> Dear Laurent,
>>>>
>>>> The "return {};" in this patch cause "maybe-uninitialized" warnings in
>>>> my CI pipeline:
>>>
>>> Thank you for the report. We have CI pipelines too, but clearly, there
>>> are way more compiler/platform combinations out there that we test at
>>> the moment :-)
>>>
>>> Could you report the compiler, compiler version and platform, for my
>>> information ?
>>
>> It's an arm64 corss compile workflow with gcc 9 [1]. See the full log at
>> [2].
>>
>> [1] https://github.com/christianrauch/libcamera-ci/blob/ci/.github/workflows/main_cross.yml
>> [2] https://github.com/christianrauch/libcamera-ci/runs/7639256594?check_suite_focus=true
>
> Could you test the following patch to see if it fixes the problem ? If
> so I'll send it with a proper commit message.
>
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index c96e99e1317c..9162e2250ed4 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -121,24 +121,24 @@ template<>
>  std::optional<bool> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "true")
>  		return true;
>  	else if (value_ == "false")
>  		return false;
>
> -	return {};
> +	return std::nullopt;
>  }
>
>  template<>
>  std::optional<int16_t> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "")
> -		return {};
> +		return std::nullopt;
>
>  	char *end;
>
> @@ -148,7 +148,7 @@ std::optional<int16_t> YamlObject::get() const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int16_t>::min() ||
>  	    value > std::numeric_limits<int16_t>::max())
> -		return {};
> +		return std::nullopt;
>
>  	return value;
>  }
> @@ -157,10 +157,10 @@ template<>
>  std::optional<uint16_t> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "")
> -		return {};
> +		return std::nullopt;
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -171,7 +171,7 @@ std::optional<uint16_t> YamlObject::get() const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return {};
> +		return std::nullopt;
>
>  	char *end;
>
> @@ -181,7 +181,7 @@ std::optional<uint16_t> YamlObject::get() const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint16_t>::min() ||
>  	    value > std::numeric_limits<uint16_t>::max())
> -		return {};
> +		return std::nullopt;
>
>  	return value;
>  }
> @@ -190,10 +190,10 @@ template<>
>  std::optional<int32_t> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "")
> -		return {};
> +		return std::nullopt;
>
>  	char *end;
>
> @@ -203,7 +203,7 @@ std::optional<int32_t> YamlObject::get() const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<int32_t>::min() ||
>  	    value > std::numeric_limits<int32_t>::max())
> -		return {};
> +		return std::nullopt;
>
>  	return value;
>  }
> @@ -212,10 +212,10 @@ template<>
>  std::optional<uint32_t> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "")
> -		return {};
> +		return std::nullopt;
>
>  	/*
>  	 * libyaml parses all scalar values as strings. When a string has
> @@ -226,7 +226,7 @@ std::optional<uint32_t> YamlObject::get() const
>  	 */
>  	std::size_t found = value_.find_first_not_of(" \t");
>  	if (found != std::string::npos && value_[found] == '-')
> -		return {};
> +		return std::nullopt;
>
>  	char *end;
>
> @@ -236,7 +236,7 @@ std::optional<uint32_t> YamlObject::get() const
>  	if ('\0' != *end || errno == ERANGE ||
>  	    value < std::numeric_limits<uint32_t>::min() ||
>  	    value > std::numeric_limits<uint32_t>::max())
> -		return {};
> +		return std::nullopt;
>
>  	return value;
>  }
> @@ -245,10 +245,10 @@ template<>
>  std::optional<double> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	if (value_ == "")
> -		return {};
> +		return std::nullopt;
>
>  	char *end;
>
> @@ -256,7 +256,7 @@ std::optional<double> YamlObject::get() const
>  	double value = std::strtod(value_.c_str(), &end);
>
>  	if ('\0' != *end || errno == ERANGE)
> -		return {};
> +		return std::nullopt;
>
>  	return value;
>  }
> @@ -265,7 +265,7 @@ template<>
>  std::optional<std::string> YamlObject::get() const
>  {
>  	if (type_ != Type::Value)
> -		return {};
> +		return std::nullopt;
>
>  	return value_;
>  }
> @@ -274,18 +274,18 @@ template<>
>  std::optional<Size> YamlObject::get() const
>  {
>  	if (type_ != Type::List)
> -		return {};
> +		return std::nullopt;
>
>  	if (list_.size() != 2)
> -		return {};
> +		return std::nullopt;
>
>  	auto width = list_[0].value->get<uint32_t>();
>  	if (!width)
> -		return {};
> +		return std::nullopt;
>
>  	auto height = list_[1].value->get<uint32_t>();
>  	if (!height)
> -		return {};
> +		return std::nullopt;
>
>  	return Size(*width, *height);
>  }
>
>
>>>> ../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp>
>>>> libcamera::YamlObject::get() const [with T = short unsigned int;
>>>> std::enable_if_t<(((((((is_same_v<bool, T> || is_same_v<double, T>) ||
>>>> is_same_v<short int, T>) || is_same_v<short unsigned int, T>) ||
>>>> is_same_v<int, T>) || is_same_v<unsigned int, T>) ||
>>>> is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>,
>>>> std::allocator<char> >, T>) || is_same_v<libcamera::Size, T>)>*
>>>> <anonymous> = 0]’:
>>>> ../src/libcamera/yaml_parser.cpp:184:11: error: ‘<anonymous>’ may be
>>>> used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>   184 |   return {};
>>>>       |           ^
>>>>
>>>> ... and a couple more in "yaml_parser.cpp".
>>>>
>>>> Since your intention is here to notify the user of invalid values,
>>>> wouldn't it be better to "return nullptr;" instead?
>>>
>>> The would be std::nullopt as the function returns a std::optional<>, but
>>> yes, that would make sense. I was expecting {} to do the same.
>>>
>>> As I can't test if this would fix the compilation warning given that I
>>> can't reproduce them, would you be able to test that change with your CI
>>> pipeline ? If std::nullopt fixes the errors, I can send a patch, or you
>>> can send one directly if you prefer.
>>>
>>>> Am 28.07.22 um 00:21 schrieb Laurent Pinchart via libcamera-devel:
>>>>> The YamlObject::get() function takes a default value and an optional
>>>>> bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
>>>>> error handling in callers.
>>>>>
>>>>> A better API is possible by dropping the default value and ok flag and
>>>>> returning an std::optional. Not only does it simplify the calls, it also
>>>>> lets callers handle errors through the standard std::optional class
>>>>> instead of the current ad-hoc mechanism.
>>>>>
>>>>> Provide a get() wrapper around std::optional::value_or() to further
>>>>> simplify callers that don't need any specific error handling.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>>>>> Tested-by: Naushir Patuck <naush@raspberrypi.com>
>>>>> ---
>>>>>  include/libcamera/internal/yaml_parser.h |   9 +-
>>>>>  src/libcamera/yaml_parser.cpp            | 138 +++++++++--------------
>>>>>  test/yaml-parser.cpp                     |  71 ++++++------
>>>>>  3 files changed, 96 insertions(+), 122 deletions(-)
>>>>>
>>>>> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
>>>>> index 064cf44381d7..61f2223223a7 100644
>>>>> --- a/include/libcamera/internal/yaml_parser.h
>>>>> +++ b/include/libcamera/internal/yaml_parser.h
>>>>> @@ -9,6 +9,7 @@
>>>>>
>>>>>  #include <iterator>
>>>>>  #include <map>
>>>>> +#include <optional>
>>>>>  #include <string>
>>>>>  #include <vector>
>>>>>
>>>>> @@ -165,7 +166,13 @@ public:
>>>>>  #else
>>>>>  	template<typename T>
>>>>>  #endif
>>>>> -	T get(const T &defaultValue, bool *ok = nullptr) const;
>>>>> +	std::optional<T> get() const;
>>>>> +
>>>>> +	template<typename T>
>>>>> +	T get(const T &defaultValue) const
>>>>> +	{
>>>>> +		return get<T>().value_or(defaultValue);
>>>>> +	}
>>>>>
>>>>>  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
>>>>>  	ListAdapter asList() const { return ListAdapter{ list_ }; }
>>>>> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
>>>>> index 5c45e44e49c3..4299f5abd38a 100644
>>>>> --- a/src/libcamera/yaml_parser.cpp
>>>>> +++ b/src/libcamera/yaml_parser.cpp
>>>>> @@ -31,12 +31,6 @@ namespace {
>>>>>  /* Empty static YamlObject as a safe result for invalid operations */
>>>>>  static const YamlObject empty;
>>>>>
>>>>> -void setOk(bool *ok, bool result)
>>>>> -{
>>>>> -	if (ok)
>>>>> -		*ok = result;
>>>>> -}
>>>>> -
>>>>>  } /* namespace */
>>>>>
>>>>>  /**
>>>>> @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
>>>>>  }
>>>>>
>>>>>  /**
>>>>> - * \fn template<typename T> YamlObject::get<T>(
>>>>> - *	const T &defaultValue, bool *ok) const
>>>>> + * \fn template<typename T> YamlObject::get<T>() const
>>>>> + * \brief Parse the YamlObject as a \a T value
>>>>> + *
>>>>> + * This function parses the value of the YamlObject as a \a T object, and
>>>>> + * returns the value. If parsing fails (usually because the YamlObject doesn't
>>>>> + * store a \a T value), std::nullopt is returned.
>>>>> + *
>>>>> + * \return The YamlObject value, or std::nullopt if parsing failed
>>>>> + */
>>>>> +
>>>>> +/**
>>>>> + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
>>>>>   * \brief Parse the YamlObject as a \a T value
>>>>>   * \param[in] defaultValue The default value when failing to parse
>>>>> - * \param[out] ok The result of whether the parse succeeded
>>>>>   *
>>>>>   * This function parses the value of the YamlObject as a \a T object, and
>>>>>   * returns the value. If parsing fails (usually because the YamlObject doesn't
>>>>> - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
>>>>> - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
>>>>> + * store a \a T value), the \a defaultValue is returned.
>>>>>   *
>>>>> - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
>>>>> - * need to know if parsing succeeded.
>>>>> - *
>>>>> - * \return Value as a bool type
>>>>> + * \return The YamlObject value, or \a defaultValue if parsing failed
>>>>>   */
>>>>>
>>>>>  #ifndef __DOXYGEN__
>>>>>
>>>>>  template<>
>>>>> -bool YamlObject::get(const bool &defaultValue, bool *ok) const
>>>>> +std::optional<bool> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	if (value_ == "true") {
>>>>> -		setOk(ok, true);
>>>>> +	if (value_ == "true")
>>>>>  		return true;
>>>>> -	} else if (value_ == "false") {
>>>>> -		setOk(ok, true);
>>>>> +	else if (value_ == "false")
>>>>>  		return false;
>>>>> -	}
>>>>>
>>>>> -	return defaultValue;
>>>>> +	return {};
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>>>>> +std::optional<int16_t> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (value_ == "")
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	char *end;
>>>>>
>>>>> @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
>>>>>  	if ('\0' != *end || errno == ERANGE ||
>>>>>  	    value < std::numeric_limits<int16_t>::min() ||
>>>>>  	    value > std::numeric_limits<int16_t>::max())
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>>>> +std::optional<uint16_t> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (value_ == "")
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	/*
>>>>>  	 * libyaml parses all scalar values as strings. When a string has
>>>>> @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>>>>  	 */
>>>>>  	std::size_t found = value_.find_first_not_of(" \t");
>>>>>  	if (found != std::string::npos && value_[found] == '-')
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	char *end;
>>>>>
>>>>> @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
>>>>>  	if ('\0' != *end || errno == ERANGE ||
>>>>>  	    value < std::numeric_limits<uint16_t>::min() ||
>>>>>  	    value > std::numeric_limits<uint16_t>::max())
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>>>>> +std::optional<int32_t> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (value_ == "")
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	char *end;
>>>>>
>>>>> @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>>>>>  	if ('\0' != *end || errno == ERANGE ||
>>>>>  	    value < std::numeric_limits<int32_t>::min() ||
>>>>>  	    value > std::numeric_limits<int32_t>::max())
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>>>> +std::optional<uint32_t> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (value_ == "")
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	/*
>>>>>  	 * libyaml parses all scalar values as strings. When a string has
>>>>> @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>>>>  	 */
>>>>>  	std::size_t found = value_.find_first_not_of(" \t");
>>>>>  	if (found != std::string::npos && value_[found] == '-')
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	char *end;
>>>>>
>>>>> @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>>>>>  	if ('\0' != *end || errno == ERANGE ||
>>>>>  	    value < std::numeric_limits<uint32_t>::min() ||
>>>>>  	    value > std::numeric_limits<uint32_t>::max())
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -double YamlObject::get(const double &defaultValue, bool *ok) const
>>>>> +std::optional<double> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (value_ == "")
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	char *end;
>>>>>
>>>>> @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
>>>>>  	double value = std::strtod(value_.c_str(), &end);
>>>>>
>>>>>  	if ('\0' != *end || errno == ERANGE)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
>>>>> +std::optional<std::string> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::Value)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>>  	return value_;
>>>>>  }
>>>>>
>>>>>  template<>
>>>>> -Size YamlObject::get(const Size &defaultValue, bool *ok) const
>>>>> +std::optional<Size> YamlObject::get() const
>>>>>  {
>>>>> -	setOk(ok, false);
>>>>> -
>>>>>  	if (type_ != Type::List)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>>  	if (list_.size() != 2)
>>>>> -		return defaultValue;
>>>>> +		return {};
>>>>>
>>>>> -	/*
>>>>> -	 * Add a local variable to validate each dimension in case
>>>>> -	 * that ok == nullptr.
>>>>> -	 */
>>>>> -	bool valid;
>>>>> -	uint32_t width = list_[0]->get<uint32_t>(0, &valid);
>>>>> -	if (!valid)
>>>>> -		return defaultValue;
>>>>> +	auto width = list_[0]->get<uint32_t>();
>>>>> +	if (!width)
>>>>> +		return {};
>>>>>
>>>>> -	uint32_t height = list_[1]->get<uint32_t>(0, &valid);
>>>>> -	if (!valid)
>>>>> -		return defaultValue;
>>>>> +	auto height = list_[1]->get<uint32_t>();
>>>>> +	if (!height)
>>>>> +		return {};
>>>>>
>>>>> -	setOk(ok, true);
>>>>> -	return Size(width, height);
>>>>> +	return Size(*width, *height);
>>>>>  }
>>>>>
>>>>>  #endif /* __DOXYGEN__ */
>>>>> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
>>>>> index 38f848232fa6..ebb654f2ef9c 100644
>>>>> --- a/test/yaml-parser.cpp
>>>>> +++ b/test/yaml-parser.cpp
>>>>> @@ -148,7 +148,6 @@ protected:
>>>>>  		}
>>>>>
>>>>>  		/* Test string object */
>>>>> -		bool ok;
>>>>>  		auto &strObj = (*root)["string"];
>>>>>
>>>>>  		if (strObj.isDictionary()) {
>>>>> @@ -161,27 +160,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
>>>>> +		if (strObj.get<string>().value_or("") != "libcamera") {
>>>>>  			cerr << "String object parse as wrong content" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>>>> +		if (strObj.get<int32_t>()) {
>>>>>  			cerr << "String object parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (strObj.get<uint32_t>()) {
>>>>>  			cerr << "String object parse as unsigned integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>>>> +		if (strObj.get<double>()) {
>>>>>  			cerr << "String object parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (strObj.get<Size>()) {
>>>>>  			cerr << "String object parse as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -199,27 +198,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
>>>>> +		if (int32Obj.get<int32_t>().value_or(0) != -100) {
>>>>>  			cerr << "Integer object parse as wrong value" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
>>>>> +		if (int32Obj.get<string>().value_or("") != "-100") {
>>>>>  			cerr << "Integer object fail to parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
>>>>> +		if (int32Obj.get<double>().value_or(0.0) != -100.0) {
>>>>>  			cerr << "Integer object fail to parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (int32Obj.get<uint32_t>()) {
>>>>>  			cerr << "Negative integer object parse as unsigned integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (int32Obj.get<Size>()) {
>>>>>  			cerr << "Integer object parse as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -237,27 +236,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
>>>>> +		if (uint32Obj.get<int32_t>().value_or(0) != 100) {
>>>>>  			cerr << "Unsigned integer object fail to parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
>>>>> +		if (uint32Obj.get<string>().value_or("") != "100") {
>>>>>  			cerr << "Unsigned integer object fail to parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
>>>>> +		if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
>>>>>  			cerr << "Unsigned integer object fail to parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
>>>>> +		if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
>>>>>  			cerr << "Unsigned integer object parsed as wrong value" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (uint32Obj.get<Size>()) {
>>>>>  			cerr << "Unsigned integer object parsed as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -275,27 +274,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
>>>>> +		if (doubleObj.get<string>().value_or("") != "3.14159") {
>>>>>  			cerr << "Double object fail to parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
>>>>> +		if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
>>>>>  			cerr << "Double object parse as wrong value" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>>>> +		if (doubleObj.get<int32_t>()) {
>>>>>  			cerr << "Double object parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (doubleObj.get<uint32_t>()) {
>>>>>  			cerr << "Double object parse as unsigned integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (doubleObj.get<Size>()) {
>>>>>  			cerr << "Double object parse as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -313,27 +312,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (sizeObj.get<string>("", &ok) != "" || ok) {
>>>>> +		if (sizeObj.get<string>()) {
>>>>>  			cerr << "Size object parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>>>> +		if (sizeObj.get<double>()) {
>>>>>  			cerr << "Size object parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>>>> +		if (sizeObj.get<int32_t>()) {
>>>>>  			cerr << "Size object parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (sizeObj.get<uint32_t>()) {
>>>>>  			cerr << "Size object parse as unsigned integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
>>>>> +		if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
>>>>>  			cerr << "Size object parse as wrong value" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -351,27 +350,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (listObj.get<string>("", &ok) != "" || ok) {
>>>>> +		if (listObj.get<string>()) {
>>>>>  			cerr << "List object parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>>>> +		if (listObj.get<double>()) {
>>>>>  			cerr << "List object parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>>>> +		if (listObj.get<int32_t>()) {
>>>>>  			cerr << "List object parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (listObj.get<uint32_t>()) {
>>>>>  			cerr << "List object parse as unsigne integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (listObj.get<Size>()) {
>>>>>  			cerr << "String list object parse as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>> @@ -424,27 +423,27 @@ protected:
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (dictObj.get<string>("", &ok) != "" || ok) {
>>>>> +		if (dictObj.get<string>()) {
>>>>>  			cerr << "Dictionary object parse as string" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
>>>>> +		if (dictObj.get<double>()) {
>>>>>  			cerr << "Dictionary object parse as double" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
>>>>> +		if (dictObj.get<int32_t>()) {
>>>>>  			cerr << "Dictionary object parse as integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
>>>>> +		if (dictObj.get<uint32_t>()) {
>>>>>  			cerr << "Dictionary object parse as unsigned integer" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>>>>>
>>>>> -		if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
>>>>> +		if (dictObj.get<Size>()) {
>>>>>  			cerr << "Dictionary object parse as Size" << std::endl;
>>>>>  			return TestFail;
>>>>>  		}
>