[PATCH] libcamera: yaml-parser: Differentiate between empty and empty string

[Laurent Pinchart]

On Tue, Sep 17, 2024 at 04:12:45PM +0200, Paul Elder wrote:
> On Tue, Sep 17, 2024 at 12:28:20AM +0200, Stefan Klug wrote:
> > When accessing a nonexistent key on a dict the YamlObject returns an
> > empty element. This element can happily be casted to a string which is

s/casted/cast/

> > unexpected. For example the following statement:
> > yamlDict["nonexistant"].get<string>("default") is expected to return

unexpected. For example the following statement:

yamlDict["nonexistent"].get<string>("default")

is expected to return

> s/nonexistant/nonexistent/
> 
> > "default" but actually returns "". Fix this by introducing a empty type
> 
> s/ a / an /
> 
> > to distinguish between an empty YamlObject and a YamlObject of type
> > value containing an empty string. For completeness add an isEmpty()
> > function to be able to test for that type.
> 
> Ooh that's a smart solution.

Indeed. Normally I'd worry that this could break things in unexpected
ways, but we have unit tests :-)

> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  include/libcamera/internal/yaml_parser.h |  5 +++++
> >  src/libcamera/yaml_parser.cpp            | 20 ++++++++++++++------
> >  test/yaml-parser.cpp                     |  6 ++++++
> >  3 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 16708e488d88..33c3a96fed47 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -159,6 +159,10 @@ public:
> >  	{
> >  		return type_ == Type::Dictionary;
> >  	}
> > +	bool isEmpty() const
> > +	{
> > +		return type_ == Type::Empty;
> > +	}
> >  
> >  	std::size_t size() const;
> >  
> > @@ -212,6 +216,7 @@ private:
> >  		Dictionary,
> >  		List,
> >  		Value,
> > +		Empty,
> >  	};
> >  
> >  	template<typename T>
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 8b6a403898be..eb72529136f7 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -38,12 +38,12 @@ static const YamlObject empty;
> >   * \brief A class representing the tree structure of the YAML content
> >   *
> >   * The YamlObject class represents the tree structure of YAML content. A
> > - * YamlObject can be a dictionary or list of YamlObjects or a value if a tree
> > - * leaf.
> > + * YamlObject can be empty, a dictionary or list of YamlObjects or a value if a

 * YamlObject can be empty, a dictionary or list of YamlObjects, or a value if a

> > + * tree leaf.
> >   */
> >  
> >  YamlObject::YamlObject()
> > -	: type_(Type::Value)
> > +	: type_(Type::Empty)
> >  {
> >  }
> >  
> > @@ -70,6 +70,13 @@ YamlObject::~YamlObject() = default;
> >   * \return True if the YamlObject is a dictionary, false otherwise
> >   */
> >  
> > +/**
> > + * \fn YamlObject::isEmpty()
> > + * \brief Return whether the YamlObject is a empty
> 
> s/ a / an /
> 
> > + *
> > + * \return True if the YamlObject is empty, false otherwise
> > + */
> > +
> >  /**
> >   * \fn YamlObject::size()
> >   * \brief Retrieve the number of elements in a dictionary or list YamlObject
> > @@ -443,7 +450,8 @@ template std::optional<std::vector<Size>> YamlObject::getList<Size>() const;
> >   *
> >   * This function retrieves an element of the YamlObject. Only YamlObject
> >   * instances of List type associate elements with index, calling this function
> > - * on other types of instances is invalid and results in undefined behaviour.
> > + * on other types of instances or with an invalid index results in an empty
> > + * object.
> >   *
> >   * \return The YamlObject as an element of the list
> >   */
> > @@ -480,8 +488,8 @@ bool YamlObject::contains(const std::string &key) const
> >   *
> >   * This function retrieve a member of a YamlObject by name. Only YamlObject
> >   * instances of Dictionary type associate elements with names, calling this
> > - * function on other types of instances is invalid and results in undefined
> > - * behaviour.
> > + * function on other types of instances or with a non existant key results in an
> 
> s/non existant/nonexistent/
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > + * empty object.
> >   *
> >   * \return The YamlObject corresponding to the \a key member
> >   */
> > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > index 81c829834667..b8ae0b4c11f8 100644
> > --- a/test/yaml-parser.cpp
> > +++ b/test/yaml-parser.cpp
> > @@ -514,6 +514,12 @@ protected:
> >  			i++;
> >  		}
> >  
> > +		/* Test access to nonexistent member */

s/member/member./

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

> > +		if (dictObj["nonexistent"].get<std::string>("default") != "default") {
> > +			cerr << "Accessing nonexistent dict fails to return default" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> >  		/* Make sure utils::map_keys() works on the adapter. */
> >  		(void)utils::map_keys(dictObj.asDict());
> >  

-- 
Regards,

Laurent Pinchart