[PATCH v2] libcamera: yaml-parser: Differentiate between empty and empty string
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 20 00:24:08 CEST 2024
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.
> @@ -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 ?
> + }
> +
> /* 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 ?
> 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.
> /* Make sure utils::map_keys() works on the adapter. */
> (void)utils::map_keys(dictObj.asDict());
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list