[libcamera-devel] [PATCH v5 2/9] libcamera: yaml_parser: Replace ok flag to get() with std::optional
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jul 30 15:20:16 CEST 2022
Hi Florian,
On Fri, Jul 29, 2022 at 09:03:19AM +0200, Florian Sylvestre wrote:
> On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel wrote:
> > On Thu, Jul 28, 2022 at 01:21:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The YamlObject::get() function takes a default value and an optional
> > > bool ok flag to handle parsing errors. This ad-hoc mechanism complicates
> > > error handling in callers.
> > >
> > > A better API is possible by dropping the default value and ok flag and
> > > returning an std::optional. Not only does it simplify the calls, it also
> > > lets callers handle errors through the standard std::optional class
> > > instead of the current ad-hoc mechanism.
> > >
> > > Provide a get() wrapper around std::optional::value_or() to further
> > > simplify callers that don't need any specific error handling.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > Tested-by: Naushir Patuck <naush at raspberrypi.com>
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > ---
> > > include/libcamera/internal/yaml_parser.h | 9 +-
> > > src/libcamera/yaml_parser.cpp | 138 +++++++++--------------
> > > test/yaml-parser.cpp | 71 ++++++------
> > > 3 files changed, 96 insertions(+), 122 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > > index 064cf44381d7..61f2223223a7 100644
> > > --- a/include/libcamera/internal/yaml_parser.h
> > > +++ b/include/libcamera/internal/yaml_parser.h
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <iterator>
> > > #include <map>
> > > +#include <optional>
> > > #include <string>
> > > #include <vector>
> > >
> > > @@ -165,7 +166,13 @@ public:
> > > #else
> > > template<typename T>
> > > #endif
> > > - T get(const T &defaultValue, bool *ok = nullptr) const;
> > > + std::optional<T> get() const;
> > > +
> > > + template<typename T>
> > > + T get(const T &defaultValue) const
> > > + {
> > > + return get<T>().value_or(defaultValue);
> > > + }
>
> I have not seen any test for this specific part (it could be great at
> least as an example).
Good point. I'll add tests.
> Also, do you see any added value to add it to the getList() function
> later on this series?
> I find more readable to have something like that:
> std::vector<uint16_t> table = tuningData[prop].getList<uint16_t>({});
> than:
> std::vector<uint16_t> table =
> tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
> in case an empty vector is not expected (and we can do check on size)
Do you mean something along the lines of
template<typename T>
std::vector<T> getList(const std::vector<T> &defaultValue);
?
> > > DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> > > ListAdapter asList() const { return ListAdapter{ list_ }; }
> > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > > index 5c45e44e49c3..4299f5abd38a 100644
> > > --- a/src/libcamera/yaml_parser.cpp
> > > +++ b/src/libcamera/yaml_parser.cpp
> > > @@ -31,12 +31,6 @@ namespace {
> > > /* Empty static YamlObject as a safe result for invalid operations */
> > > static const YamlObject empty;
> > >
> > > -void setOk(bool *ok, bool result)
> > > -{
> > > - if (ok)
> > > - *ok = result;
> > > -}
> > > -
> > > } /* namespace */
> > >
> > > /**
> > > @@ -100,54 +94,52 @@ std::size_t YamlObject::size() const
> > > }
> > >
> > > /**
> > > - * \fn template<typename T> YamlObject::get<T>(
> > > - * const T &defaultValue, bool *ok) const
> > > + * \fn template<typename T> YamlObject::get<T>() const
> > > + * \brief Parse the YamlObject as a \a T value
> > > + *
> > > + * This function parses the value of the YamlObject as a \a T object, and
> > > + * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > + * store a \a T value), std::nullopt is returned.
> > > + *
> > > + * \return The YamlObject value, or std::nullopt if parsing failed
> > > + */
> > > +
> > > +/**
> > > + * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const
> > > * \brief Parse the YamlObject as a \a T value
> > > * \param[in] defaultValue The default value when failing to parse
> > > - * \param[out] ok The result of whether the parse succeeded
> > > *
> > > * This function parses the value of the YamlObject as a \a T object, and
> > > * returns the value. If parsing fails (usually because the YamlObject doesn't
> > > - * store a \a T value), the \a defaultValue is returned, and \a ok is set to
> > > - * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.
> > > + * store a \a T value), the \a defaultValue is returned.
> > > *
> > > - * The \a ok pointer is optional and can be a nullptr if the caller doesn't
> > > - * need to know if parsing succeeded.
> > > - *
> > > - * \return Value as a bool type
> > > + * \return The YamlObject value, or \a defaultValue if parsing failed
> > > */
> > >
> > > #ifndef __DOXYGEN__
> > >
> > > template<>
> > > -bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > > +std::optional<bool> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > - if (value_ == "true") {
> > > - setOk(ok, true);
> > > + if (value_ == "true")
> > > return true;
> > > - } else if (value_ == "false") {
> > > - setOk(ok, true);
> > > + else if (value_ == "false")
> > > return false;
> > > - }
> > >
> > > - return defaultValue;
> > > + return {};
> > > }
> > >
> > > template<>
> > > -int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > +std::optional<int16_t> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (value_ == "")
> > > - return defaultValue;
> > > + return {};
> > >
> > > char *end;
> > >
> > > @@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const
> > > if ('\0' != *end || errno == ERANGE ||
> > > value < std::numeric_limits<int16_t>::min() ||
> > > value > std::numeric_limits<int16_t>::max())
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value;
> > > }
> > >
> > > template<>
> > > -uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > +std::optional<uint16_t> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (value_ == "")
> > > - return defaultValue;
> > > + return {};
> > >
> > > /*
> > > * libyaml parses all scalar values as strings. When a string has
> > > @@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > */
> > > std::size_t found = value_.find_first_not_of(" \t");
> > > if (found != std::string::npos && value_[found] == '-')
> > > - return defaultValue;
> > > + return {};
> > >
> > > char *end;
> > >
> > > @@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const
> > > if ('\0' != *end || errno == ERANGE ||
> > > value < std::numeric_limits<uint16_t>::min() ||
> > > value > std::numeric_limits<uint16_t>::max())
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value;
> > > }
> > >
> > > template<>
> > > -int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > +std::optional<int32_t> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (value_ == "")
> > > - return defaultValue;
> > > + return {};
> > >
> > > char *end;
> > >
> > > @@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > > if ('\0' != *end || errno == ERANGE ||
> > > value < std::numeric_limits<int32_t>::min() ||
> > > value > std::numeric_limits<int32_t>::max())
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value;
> > > }
> > >
> > > template<>
> > > -uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > +std::optional<uint32_t> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (value_ == "")
> > > - return defaultValue;
> > > + return {};
> > >
> > > /*
> > > * libyaml parses all scalar values as strings. When a string has
> > > @@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > */
> > > std::size_t found = value_.find_first_not_of(" \t");
> > > if (found != std::string::npos && value_[found] == '-')
> > > - return defaultValue;
> > > + return {};
> > >
> > > char *end;
> > >
> > > @@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > > if ('\0' != *end || errno == ERANGE ||
> > > value < std::numeric_limits<uint32_t>::min() ||
> > > value > std::numeric_limits<uint32_t>::max())
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value;
> > > }
> > >
> > > template<>
> > > -double YamlObject::get(const double &defaultValue, bool *ok) const
> > > +std::optional<double> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (value_ == "")
> > > - return defaultValue;
> > > + return {};
> > >
> > > char *end;
> > >
> > > @@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const
> > > double value = std::strtod(value_.c_str(), &end);
> > >
> > > if ('\0' != *end || errno == ERANGE)
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value;
> > > }
> > >
> > > template<>
> > > -std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > > +std::optional<std::string> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::Value)
> > > - return defaultValue;
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > return value_;
> > > }
> > >
> > > template<>
> > > -Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > > +std::optional<Size> YamlObject::get() const
> > > {
> > > - setOk(ok, false);
> > > -
> > > if (type_ != Type::List)
> > > - return defaultValue;
> > > + return {};
> > >
> > > if (list_.size() != 2)
> > > - return defaultValue;
> > > + return {};
> > >
> > > - /*
> > > - * Add a local variable to validate each dimension in case
> > > - * that ok == nullptr.
> > > - */
> > > - bool valid;
> > > - uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > > - if (!valid)
> > > - return defaultValue;
> > > + auto width = list_[0]->get<uint32_t>();
> > > + if (!width)
> > > + return {};
> > >
> > > - uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > > - if (!valid)
> > > - return defaultValue;
> > > + auto height = list_[1]->get<uint32_t>();
> > > + if (!height)
> > > + return {};
> > >
> > > - setOk(ok, true);
> > > - return Size(width, height);
> > > + return Size(*width, *height);
> > > }
> > >
> > > #endif /* __DOXYGEN__ */
> > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > index 38f848232fa6..ebb654f2ef9c 100644
> > > --- a/test/yaml-parser.cpp
> > > +++ b/test/yaml-parser.cpp
> > > @@ -148,7 +148,6 @@ protected:
> > > }
> > >
> > > /* Test string object */
> > > - bool ok;
> > > auto &strObj = (*root)["string"];
> > >
> > > if (strObj.isDictionary()) {
> > > @@ -161,27 +160,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (strObj.get<string>("", &ok) != "libcamera" || !ok) {
> > > + if (strObj.get<string>().value_or("") != "libcamera") {
> > > cerr << "String object parse as wrong content" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > + if (strObj.get<int32_t>()) {
> > > cerr << "String object parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (strObj.get<uint32_t>()) {
> > > cerr << "String object parse as unsigned integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > + if (strObj.get<double>()) {
> > > cerr << "String object parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (strObj.get<Size>()) {
> > > cerr << "String object parse as Size" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -199,27 +198,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {
> > > + if (int32Obj.get<int32_t>().value_or(0) != -100) {
> > > cerr << "Integer object parse as wrong value" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (int32Obj.get<string>("", &ok) != "-100" || !ok) {
> > > + if (int32Obj.get<string>().value_or("") != "-100") {
> > > cerr << "Integer object fail to parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {
> > > + if (int32Obj.get<double>().value_or(0.0) != -100.0) {
> > > cerr << "Integer object fail to parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (int32Obj.get<uint32_t>()) {
> > > cerr << "Negative integer object parse as unsigned integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (int32Obj.get<Size>()) {
> > > cerr << "Integer object parse as Size" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -237,27 +236,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {
> > > + if (uint32Obj.get<int32_t>().value_or(0) != 100) {
> > > cerr << "Unsigned integer object fail to parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (uint32Obj.get<string>("", &ok) != "100" || !ok) {
> > > + if (uint32Obj.get<string>().value_or("") != "100") {
> > > cerr << "Unsigned integer object fail to parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {
> > > + if (uint32Obj.get<double>().value_or(0.0) != 100.0) {
> > > cerr << "Unsigned integer object fail to parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {
> > > + if (uint32Obj.get<uint32_t>().value_or(0) != 100) {
> > > cerr << "Unsigned integer object parsed as wrong value" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (uint32Obj.get<Size>()) {
> > > cerr << "Unsigned integer object parsed as Size" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -275,27 +274,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {
> > > + if (doubleObj.get<string>().value_or("") != "3.14159") {
> > > cerr << "Double object fail to parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {
> > > + if (doubleObj.get<double>().value_or(0.0) != 3.14159) {
> > > cerr << "Double object parse as wrong value" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > + if (doubleObj.get<int32_t>()) {
> > > cerr << "Double object parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (doubleObj.get<uint32_t>()) {
> > > cerr << "Double object parse as unsigned integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (doubleObj.get<Size>()) {
> > > cerr << "Double object parse as Size" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -313,27 +312,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (sizeObj.get<string>("", &ok) != "" || ok) {
> > > + if (sizeObj.get<string>()) {
> > > cerr << "Size object parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > + if (sizeObj.get<double>()) {
> > > cerr << "Size object parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > + if (sizeObj.get<int32_t>()) {
> > > cerr << "Size object parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (sizeObj.get<uint32_t>()) {
> > > cerr << "Size object parse as unsigned integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {
> > > + if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {
> > > cerr << "Size object parse as wrong value" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -351,27 +350,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (listObj.get<string>("", &ok) != "" || ok) {
> > > + if (listObj.get<string>()) {
> > > cerr << "List object parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > + if (listObj.get<double>()) {
> > > cerr << "List object parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > + if (listObj.get<int32_t>()) {
> > > cerr << "List object parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (listObj.get<uint32_t>()) {
> > > cerr << "List object parse as unsigne integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (listObj.get<Size>()) {
> > > cerr << "String list object parse as Size" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -424,27 +423,27 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (dictObj.get<string>("", &ok) != "" || ok) {
> > > + if (dictObj.get<string>()) {
> > > cerr << "Dictionary object parse as string" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {
> > > + if (dictObj.get<double>()) {
> > > cerr << "Dictionary object parse as double" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {
> > > + if (dictObj.get<int32_t>()) {
> > > cerr << "Dictionary object parse as integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {
> > > + if (dictObj.get<uint32_t>()) {
> > > cerr << "Dictionary object parse as unsigned integer" << std::endl;
> > > return TestFail;
> > > }
> > >
> > > - if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {
> > > + if (dictObj.get<Size>()) {
> > > cerr << "Dictionary object parse as Size" << std::endl;
> > > return TestFail;
> > > }
>
> Reviewed-by: Florian Sylvestre <fsylvestre at baylibre.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list