| Message ID | 20240919141832.40249-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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()); >
Hi Laurent, Thank you for the review. On Fri, Sep 20, 2024 at 01:24:08AM +0300, Laurent Pinchart wrote: > 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@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. I thought about that before. Yes that would have made it clearer. I'll changes that. > > > @@ -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 ? Oh missed that. Thanks. > > > + } > > + > > /* 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 ? The idea was to have a explicit sentinel value, that will never occur by a failed string to int conversion or something returning 0 as fallback. And having 0 in dictValues seems not right. The -100 is equally wrong but it seems clearer to the reader that that was on purpose. I can change that back to 0 if you like - up to you. > > > 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. Yes, I'll do that. Regards, Stefan > > > /* Make sure utils::map_keys() works on the adapter. */ > > (void)utils::map_keys(dictObj.asDict()); > > > > -- > Regards, > > Laurent Pinchart
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 81c829834667..9d340e29d4a9 100644 --- a/test/yaml-parser.cpp +++ b/test/yaml-parser.cpp @@ -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; + } + /* 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) { 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. */ + 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; + } + /* Make sure utils::map_keys() works on the adapter. */ (void)utils::map_keys(dictObj.asDict());
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@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(-)