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

[Paul Elder]

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
> unexpected. For example the following statement:
> yamlDict["nonexistant"].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.

> 
> 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
> + * 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 */
> +		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());
>  
> -- 
> 2.43.0
>