libcamera: yaml-parser: Differentiate between empty and empty string

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 an 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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

4 files changed, 49 insertions, 7 deletions

diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
index 16708e48..6211ff4a 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 8b6a4038..4784f2dc 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)
 {
 }
 
@@ -71,6 +71,20 @@ YamlObject::~YamlObject() = default;
  */
 
 /**
+ * \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/meson.build b/test/meson.build
index dcd169a8..5ed052ed 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -73,7 +73,7 @@ internal_tests = [
     {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},
     {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},
     {'name': 'utils', 'sources': ['utils.cpp']},
-    {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp'], 'should_fail': true},
+    {'name': 'yaml-parser', 'sources': ['yaml-parser.cpp']},
 ]
 
 internal_non_parallel_tests = [
diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
index 4cc77e26..1b22c87b 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());