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

[Kieran Bingham]

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 at ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> Tested-by: Naushir Patuck <naush at 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;
>                 }