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

[Paul Elder]

On Fri, Sep 20, 2024 at 03:28:10PM +0200, Stefan Klug wrote:
> When accessing a nonexistent key on a dict the YamlObject returns an
> empty element. This element can happily be cast to a string which is
> unexpected. For example the following statement:
> 
> yamlDict["nonexistent"].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.
> 
> Extend the tests accordingly.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>

You should either squash this with 2/3 or add the should_fail like Kieran
suggested to avoid bisection failures.

But otherwise,

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> 
> ---
> Changes in v3:
> - Move tests for the existing functionality into own patches
> - Fixed some typos
> 
> 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                     | 18 ++++++++++++++++
>  3 files changed, 48 insertions(+), 6 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
>   */
> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> index 4cc77e26ae39..1b22c87b72f1 100644
> --- a/test/yaml-parser.cpp
> +++ b/test/yaml-parser.cpp
> @@ -542,6 +542,24 @@ protected:
>  			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 explicit cast to bool on an empty object returns true. */
> +		if (!!dictObj["empty"] != true) {
> +			cerr << "Casting empty entry to bool returns false" << 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;
> +		}
> +
>  		/* Make sure utils::map_keys() works on the adapter. */
>  		(void)utils::map_keys(dictObj.asDict());
>  
> -- 
> 2.43.0
>