[PATCH v3 3/3] libcamera: yaml-parser: Differentiate between empty and empty string
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Sep 21 14:42:57 CEST 2024
Quoting Stefan Klug (2024-09-20 14:28:10)
> 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
s/introducing a empty/introducing an empty/
> 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>
>
> ---
> 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;
I presume this could also be :
return !isEmpty();
But there's no harm in open coding here too.
> + }
>
> 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
> + *
I'm stumped on 'is an empty' here ... but I actually think it could be
correct as it is an 'empty' object. Not that the object is empty ... ;-)
And that sounds like it means all the same but somehow there seems to be
a distinction in my head.
Anyway, I think it's fine.
> + * \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());
>
And again, now that the tests pass here - adding the following hunk to
'undo' the addition proposed in 2/3:
diff --git a/test/meson.build b/test/meson.build
index dcd169a8793e..5ed052ed62c8 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 = [
And then:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> --
> 2.43.0
>
More information about the libcamera-devel
mailing list