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

[Laurent Pinchart]

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 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") {
> 		  }

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;
> >                 }

-- 
Regards,

Laurent Pinchart