[libcamera-devel] [PATCH v5 2/9] libcamera: yaml_parser: Replace ok flag to get() with std::optional
Florian Sylvestre
fsylvestre at baylibre.com
Fri Jul 29 09:03:19 CEST 2022
Hi Laurent,
On Thu, 28 Jul 2022 at 09:00, Jacopo Mondi via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Laurent
>
> 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>
>
> Thanks
> j
>
> > ---
> > 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).
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)
> > 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;
> > }
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
Reviewed-by: Florian Sylvestre <fsylvestre at baylibre.com>
--
Florian Sylvestre
More information about the libcamera-devel
mailing list