| Message ID | 20240916222834.464460-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. szeptember 17., kedd 00:28 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta: > 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 to be able to test for that type. > > Signed-off-by: Stefan Klug <stefan.klug@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; > + } I think `constexpr explicit operator bool() const` would be a good addition as well. Then you can just do e.g. if (auto y = x["something"]) { // ... } > > std::size_t size() const; > > @@ -212,6 +216,7 @@ private: > Dictionary, > List, > Value, > + Empty, > }; > [...] Regards, Barnabás Pőcze
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@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@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 >
On Tue, Sep 17, 2024 at 04:12:45PM +0200, Paul Elder wrote: > 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 s/casted/cast/ > > unexpected. For example the following statement: > > yamlDict["nonexistant"].get<string>("default") is expected to return unexpected. For example the following statement: yamlDict["nonexistent"].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. Indeed. Normally I'd worry that this could break things in unexpected ways, but we have unit tests :-) > > Signed-off-by: Stefan Klug <stefan.klug@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 * 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@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 */ s/member/member./ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + 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()); > >
Hi Barnabás, On Tue, Sep 17, 2024 at 01:45:13PM +0000, Barnabás Pőcze wrote: > Hi > > > 2024. szeptember 17., kedd 00:28 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta: > > > 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 to be able to test for that type. > > > > Signed-off-by: Stefan Klug <stefan.klug@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; > > + } > > I think `constexpr explicit operator bool() const` would be a good addition as well. > Then you can just do e.g. > > if (auto y = x["something"]) { > // ... > } Thanks for the suggestion. I'll add that. One question though. Why the constexpr? I can't come up with a practical case were that can be resolved at compiletime. Best regards, Stefan > > > > > > std::size_t size() const; > > > > @@ -212,6 +216,7 @@ private: > > Dictionary, > > List, > > Value, > > + Empty, > > }; > > [...] > > > Regards, > Barnabás Pőcze
Hi 2024. szeptember 18., szerda 11:25 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta: > Hi Barnabás, > > On Tue, Sep 17, 2024 at 01:45:13PM +0000, Barnabás Pőcze wrote: > > Hi > > > > > > 2024. szeptember 17., kedd 00:28 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta: > > > > > 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 to be able to test for that type. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@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; > > > + } > > > > I think `constexpr explicit operator bool() const` would be a good addition as well. > > Then you can just do e.g. > > > > if (auto y = x["something"]) { > > // ... > > } > > Thanks for the suggestion. I'll add that. One question though. Why the > constexpr? I can't come up with a practical case were that can be > resolved at compiletime. I did it mostly automatically, so I guess you're right. Habit is second nature... Regards, Barnabás Pőcze
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 + * + * \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 + * 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());
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 to be able to test for that type. Signed-off-by: Stefan Klug <stefan.klug@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(-)