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

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Fri, Sep 20, 2024 at 01:24:08AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thu, Sep 19, 2024 at 04:17:41PM +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
> > unexpected. For example the following statement:
> > 
> > yamlDict["nonexistant"].get<string>("default")
> > 
> > is expected to return "default" but actually returns "". Fix this by
> > introducing a empty type to distinguish between an empty YamlObject and
> > a YamlObject of type value containing an empty string. For completeness
> > add an isEmpty() function and an explicit cast to bool to be able to
> > test for that type.
> > 
> > The tests are adapted accordingly. Additional tests for empty entries in
> > lists and dicts are added to ensure that these still get parsed as empty
> > strings. This is needed for algorithms without parameters to be parsed
> > correctly from tuning files.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - Added explicit to bool conversion operator
> > - Fixed typos from review
> > - I didn't add the rb tags, because the v2 adds quite some logic
> >   (especially tests) and this is a central piece of code
> > ---
> >  include/libcamera/internal/yaml_parser.h |  9 +++++
> >  src/libcamera/yaml_parser.cpp            | 27 ++++++++++---
> >  test/yaml-parser.cpp                     | 51 ++++++++++++++++++++++--
> >  3 files changed, 78 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 16708e488d88..6211ff4ae563 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -159,6 +159,14 @@ public:
> >  	{
> >  		return type_ == Type::Dictionary;
> >  	}
> > +	bool isEmpty() const
> > +	{
> > +		return type_ == Type::Empty;
> > +	}
> > +	explicit operator bool() const
> > +	{
> > +		return type_ != Type::Empty;
> > +	}
> >  
> >  	std::size_t size() const;
> >  
> > @@ -212,6 +220,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..4784f2dc3d62 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
> > + * tree leaf.
> >   */
> >  
> >  YamlObject::YamlObject()
> > -	: type_(Type::Value)
> > +	: type_(Type::Empty)
> >  {
> >  }
> >  
> > @@ -70,6 +70,20 @@ YamlObject::~YamlObject() = default;
> >   * \return True if the YamlObject is a dictionary, false otherwise
> >   */
> >  
> > +/**
> > + * \fn YamlObject::isEmpty()
> > + * \brief Return whether the YamlObject is an empty
> > + *
> > + * \return True if the YamlObject is empty, false otherwise
> > + */
> > +
> > +/**
> > + * \fn YamlObject::operator bool()
> > + * \brief Return whether the YamlObject is a non-empty
> > + *
> > + * \return False if the YamlObject is empty, true otherwise
> > + */
> > +
> >  /**
> >   * \fn YamlObject::size()
> >   * \brief Retrieve the number of elements in a dictionary or list YamlObject
> > @@ -443,7 +457,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 +495,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 nonexistent key results in an
> > + * empty object.
> >   *
> >   * \return The YamlObject corresponding to the \a key member
> >   */
> 
> This part looks good to me.
> 
> > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > index 81c829834667..9d340e29d4a9 100644
> > --- a/test/yaml-parser.cpp
> > +++ b/test/yaml-parser.cpp
> 
> Could you split this in a separate patch that goes *first* in the series
> ? The newly introduced tests should fail, and then the second patch
> should fix them. This helps ensuring that your tests actually test what
> you think they test.

I thought about that before. Yes that would have made it clearer. I'll
changes that.

> 
> > @@ -34,10 +34,12 @@ static const string testYaml =
> >  	"list:\n"
> >  	"  - James\n"
> >  	"  - Mary\n"
> > +	"  - \n"
> >  	"dictionary:\n"
> >  	"  a: 1\n"
> >  	"  c: 3\n"
> >  	"  b: 2\n"
> > +	"  empty:\n"
> >  	"level1:\n"
> >  	"  level2:\n"
> >  	"    - [1, 2]\n"
> > @@ -430,9 +432,10 @@ protected:
> >  		if (testObjectType(listObj, "list", Type::List) != TestPass)
> >  			return TestFail;
> >  
> > -		static constexpr std::array<const char *, 2> listValues{
> > +		static constexpr std::array<const char *, 3> listValues{
> >  			"James",
> >  			"Mary",
> > +			"",
> >  		};
> >  
> >  		if (listObj.size() != listValues.size()) {
> > @@ -465,16 +468,22 @@ protected:
> >  			i++;
> >  		}
> >  
> > +		/* Ensure that empty objects get parsed as empty strings. */
> > +		if (!listObj[2].isValue()) {
> > +			cerr << "Empty object is not a value" << std::endl;
> 
> Shouldn't this return TestFail ?

Oh missed that. Thanks.

> 
> > +		}
> > +
> >  		/* Test dictionary object */
> >  		auto &dictObj = (*root)["dictionary"];
> >  
> >  		if (testObjectType(dictObj, "dictionary", Type::Dictionary) != TestPass)
> >  			return TestFail;
> >  
> > -		static constexpr std::array<std::pair<const char *, int>, 3> dictValues{ {
> > +		static constexpr std::array<std::pair<const char *, int>, 4> dictValues{ {
> >  			{ "a", 1 },
> >  			{ "c", 3 },
> >  			{ "b", 2 },
> > +			{ "empty", -100 },
> >  		} };
> >  
> >  		size_t dictSize = dictValues.size();
> > @@ -505,7 +514,7 @@ protected:
> >  				return TestFail;
> >  			}
> >  
> > -			if (elem.get<int32_t>(0) != item.second) {
> > +			if (elem.get<int32_t>(-100) != item.second) {
> 
> Is this needed, can't you keep 0 as the default value (and update
> dictValues) accordingly ?

The idea was to have a explicit sentinel value, that will never occur by
a failed string to int conversion or something returning 0 as fallback.
And having 0 in dictValues seems not right. The -100 is equally wrong
but it seems clearer to the reader that that was on purpose. I can
change that back to 0 if you like - up to you. 

> 
> >  				std::cerr << "Dictionary element " << i << " has wrong value"
> >  					  << std::endl;
> >  				return TestFail;
> > @@ -514,6 +523,42 @@ protected:
> >  			i++;
> >  		}
> >  
> > +		/* Ensure that empty objects get parsed as empty strings. */
> > +		if (!dictObj["empty"].isValue()) {
> > +			cerr << "Empty object is not of type value" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Ensure that keys without values are still added to a dict. */
> 
> s/still //
> 
> > +		if (!dictObj.contains("empty")) {
> > +			cerr << "Empty element is missing in dict" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test explicit cast to bool on an empty object must return true. */
> > +		if (!!dictObj["empty"] != true) {
> > +			cerr << "Casting empty entry to bool returns false" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test nonexistent object has value type empty. */
> > +		if (!dictObj["nonexistent"].isEmpty()) {
> > +			cerr << "Accessing nonexistent object returns non-empty object" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test access to nonexistent member. */
> > +		if (dictObj["nonexistent"].get<std::string>("default") != "default") {
> > +			cerr << "Accessing nonexistent dict entry fails to return default" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Test explicit cast to bool on nonexistent object returns false. */
> > +		if (!!dictObj["nonexistent"] != false) {
> > +			cerr << "Casting nonexistent dict entry to bool returns true" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> 
> I would split this new test in two, with one patch for all the tests
> that currently succeed, and one for the test that fails.

Yes, I'll do that.

Regards,
Stefan

> 
> >  		/* Make sure utils::map_keys() works on the adapter. */
> >  		(void)utils::map_keys(dictObj.asDict());
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart