<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 27 Jul 2022 at 03:38, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The YamlObject::get() function takes a default value and an optional<br>
bool ok flag to handle parsing errors. This ad-hoc mechanism complicates<br>
error handling in callers.<br>
<br>
A better API is possible by dropping the default value and ok flag and<br>
returning an std::optional. Not only does it simplify the calls, it also<br>
lets callers handle errors through the standard std::optional class<br>
instead of the current ad-hoc mechanism.<br>
<br>
Provide a convenience get() wrapper around std::optional::value_or() to<br>
further simplify callers that don't need any specific error handling.<br></blockquote><div><br></div><div>s/convenience/convenient/ ?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br></blockquote><div><br></div><div>This is much cleaner for error handling!</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
include/libcamera/internal/yaml_parser.h | 9 +-<br>
src/libcamera/yaml_parser.cpp | 138 +++++++++--------------<br>
test/yaml-parser.cpp | 71 ++++++------<br>
3 files changed, 96 insertions(+), 122 deletions(-)<br>
<br>
diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h<br>
index 064cf44381d7..61f2223223a7 100644<br>
--- a/include/libcamera/internal/yaml_parser.h<br>
+++ b/include/libcamera/internal/yaml_parser.h<br>
@@ -9,6 +9,7 @@<br>
<br>
#include <iterator><br>
#include <map><br>
+#include <optional><br>
#include <string><br>
#include <vector><br>
<br>
@@ -165,7 +166,13 @@ public:<br>
#else<br>
template<typename T><br>
#endif<br>
- T get(const T &defaultValue, bool *ok = nullptr) const;<br>
+ std::optional<T> get() const;<br>
+<br>
+ template<typename T><br>
+ T get(const T &defaultValue) const<br>
+ {<br>
+ return get<T>().value_or(defaultValue);<br>
+ }<br>
<br>
DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }<br>
ListAdapter asList() const { return ListAdapter{ list_ }; }<br>
diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp<br>
index 5c45e44e49c3..4299f5abd38a 100644<br>
--- a/src/libcamera/yaml_parser.cpp<br>
+++ b/src/libcamera/yaml_parser.cpp<br>
@@ -31,12 +31,6 @@ namespace {<br>
/* Empty static YamlObject as a safe result for invalid operations */<br>
static const YamlObject empty;<br>
<br>
-void setOk(bool *ok, bool result)<br>
-{<br>
- if (ok)<br>
- *ok = result;<br>
-}<br>
-<br>
} /* namespace */<br>
<br>
/**<br>
@@ -100,54 +94,52 @@ std::size_t YamlObject::size() const<br>
}<br>
<br>
/**<br>
- * \fn template<typename T> YamlObject::get<T>(<br>
- * const T &defaultValue, bool *ok) const<br>
+ * \fn template<typename T> YamlObject::get<T>() const<br>
+ * \brief Parse the YamlObject as a \a T value<br>
+ *<br>
+ * This function parses the value of the YamlObject as a \a T object, and<br>
+ * returns the value. If parsing fails (usually because the YamlObject doesn't<br>
+ * store a \a T value), std::nullopt is returned.<br>
+ *<br>
+ * \return The YamlObject value, or std::nullopt if parsing failed<br>
+ */<br>
+<br>
+/**<br>
+ * \fn template<typename T> YamlObject::get<T>(const T &defaultValue) const<br>
* \brief Parse the YamlObject as a \a T value<br>
* \param[in] defaultValue The default value when failing to parse<br>
- * \param[out] ok The result of whether the parse succeeded<br>
*<br>
* This function parses the value of the YamlObject as a \a T object, and<br>
* returns the value. If parsing fails (usually because the YamlObject doesn't<br>
- * store a \a T value), the \a defaultValue is returned, and \a ok is set to<br>
- * false. Otherwise, the YamlObject value is returned, and \a ok is set to true.<br>
+ * store a \a T value), the \a defaultValue is returned.<br>
*<br>
- * The \a ok pointer is optional and can be a nullptr if the caller doesn't<br>
- * need to know if parsing succeeded.<br>
- *<br>
- * \return Value as a bool type<br>
+ * \return The YamlObject value, or \a defaultValue if parsing failed<br>
*/<br>
<br>
#ifndef __DOXYGEN__<br>
<br>
template<><br>
-bool YamlObject::get(const bool &defaultValue, bool *ok) const<br>
+std::optional<bool> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- if (value_ == "true") {<br>
- setOk(ok, true);<br>
+ if (value_ == "true")<br>
return true;<br>
- } else if (value_ == "false") {<br>
- setOk(ok, true);<br>
+ else if (value_ == "false")<br>
return false;<br>
- }<br>
<br>
- return defaultValue;<br>
+ return {};<br>
}<br>
<br>
template<><br>
-int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const<br>
+std::optional<int16_t> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (value_ == "")<br>
- return defaultValue;<br>
+ return {};<br>
<br>
char *end;<br>
<br>
@@ -157,22 +149,19 @@ int16_t YamlObject::get(const int16_t &defaultValue, bool *ok) const<br>
if ('\0' != *end || errno == ERANGE ||<br>
value < std::numeric_limits<int16_t>::min() ||<br>
value > std::numeric_limits<int16_t>::max())<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value;<br>
}<br>
<br>
template<><br>
-uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const<br>
+std::optional<uint16_t> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (value_ == "")<br>
- return defaultValue;<br>
+ return {};<br>
<br>
/*<br>
* libyaml parses all scalar values as strings. When a string has<br>
@@ -183,7 +172,7 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const<br>
*/<br>
std::size_t found = value_.find_first_not_of(" \t");<br>
if (found != std::string::npos && value_[found] == '-')<br>
- return defaultValue;<br>
+ return {};<br>
<br>
char *end;<br>
<br>
@@ -193,22 +182,19 @@ uint16_t YamlObject::get(const uint16_t &defaultValue, bool *ok) const<br>
if ('\0' != *end || errno == ERANGE ||<br>
value < std::numeric_limits<uint16_t>::min() ||<br>
value > std::numeric_limits<uint16_t>::max())<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value;<br>
}<br>
<br>
template<><br>
-int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const<br>
+std::optional<int32_t> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (value_ == "")<br>
- return defaultValue;<br>
+ return {};<br>
<br>
char *end;<br>
<br>
@@ -218,22 +204,19 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const<br>
if ('\0' != *end || errno == ERANGE ||<br>
value < std::numeric_limits<int32_t>::min() ||<br>
value > std::numeric_limits<int32_t>::max())<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value;<br>
}<br>
<br>
template<><br>
-uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const<br>
+std::optional<uint32_t> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (value_ == "")<br>
- return defaultValue;<br>
+ return {};<br>
<br>
/*<br>
* libyaml parses all scalar values as strings. When a string has<br>
@@ -244,7 +227,7 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const<br>
*/<br>
std::size_t found = value_.find_first_not_of(" \t");<br>
if (found != std::string::npos && value_[found] == '-')<br>
- return defaultValue;<br>
+ return {};<br>
<br>
char *end;<br>
<br>
@@ -254,22 +237,19 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const<br>
if ('\0' != *end || errno == ERANGE ||<br>
value < std::numeric_limits<uint32_t>::min() ||<br>
value > std::numeric_limits<uint32_t>::max())<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value;<br>
}<br>
<br>
template<><br>
-double YamlObject::get(const double &defaultValue, bool *ok) const<br>
+std::optional<double> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (value_ == "")<br>
- return defaultValue;<br>
+ return {};<br>
<br>
char *end;<br>
<br>
@@ -277,50 +257,38 @@ double YamlObject::get(const double &defaultValue, bool *ok) const<br>
double value = std::strtod(value_.c_str(), &end);<br>
<br>
if ('\0' != *end || errno == ERANGE)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value;<br>
}<br>
<br>
template<><br>
-std::string YamlObject::get(const std::string &defaultValue, bool *ok) const<br>
+std::optional<std::string> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::Value)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
return value_;<br>
}<br>
<br>
template<><br>
-Size YamlObject::get(const Size &defaultValue, bool *ok) const<br>
+std::optional<Size> YamlObject::get() const<br>
{<br>
- setOk(ok, false);<br>
-<br>
if (type_ != Type::List)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
if (list_.size() != 2)<br>
- return defaultValue;<br>
+ return {};<br>
<br>
- /*<br>
- * Add a local variable to validate each dimension in case<br>
- * that ok == nullptr.<br>
- */<br>
- bool valid;<br>
- uint32_t width = list_[0]->get<uint32_t>(0, &valid);<br>
- if (!valid)<br>
- return defaultValue;<br>
+ auto width = list_[0]->get<uint32_t>();<br>
+ if (!width)<br>
+ return {};<br>
<br>
- uint32_t height = list_[1]->get<uint32_t>(0, &valid);<br>
- if (!valid)<br>
- return defaultValue;<br>
+ auto height = list_[1]->get<uint32_t>();<br>
+ if (!height)<br>
+ return {};<br>
<br>
- setOk(ok, true);<br>
- return Size(width, height);<br>
+ return Size(*width, *height);<br>
}<br>
<br>
#endif /* __DOXYGEN__ */<br>
diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp<br>
index 38f848232fa6..ebb654f2ef9c 100644<br>
--- a/test/yaml-parser.cpp<br>
+++ b/test/yaml-parser.cpp<br>
@@ -148,7 +148,6 @@ protected:<br>
}<br>
<br>
/* Test string object */<br>
- bool ok;<br>
auto &strObj = (*root)["string"];<br>
<br>
if (strObj.isDictionary()) {<br>
@@ -161,27 +160,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (strObj.get<string>("", &ok) != "libcamera" || !ok) {<br>
+ if (strObj.get<string>().value_or("") != "libcamera") {<br>
cerr << "String object parse as wrong content" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (strObj.get<int32_t>(-1, &ok) != -1 || ok) {<br>
+ if (strObj.get<int32_t>()) {<br>
cerr << "String object parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (strObj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (strObj.get<uint32_t>()) {<br>
cerr << "String object parse as unsigned integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (strObj.get<double>(1.0, &ok) != 1.0 || ok) {<br>
+ if (strObj.get<double>()) {<br>
cerr << "String object parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (strObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (strObj.get<Size>()) {<br>
cerr << "String object parse as Size" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -199,27 +198,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (int32Obj.get<int32_t>(-100, &ok) != -100 || !ok) {<br>
+ if (int32Obj.get<int32_t>().value_or(0) != -100) {<br>
cerr << "Integer object parse as wrong value" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (int32Obj.get<string>("", &ok) != "-100" || !ok) {<br>
+ if (int32Obj.get<string>().value_or("") != "-100") {<br>
cerr << "Integer object fail to parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (int32Obj.get<double>(1.0, &ok) != -100.0 || !ok) {<br>
+ if (int32Obj.get<double>().value_or(0.0) != -100.0) {<br>
cerr << "Integer object fail to parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (int32Obj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (int32Obj.get<uint32_t>()) {<br>
cerr << "Negative integer object parse as unsigned integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (int32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (int32Obj.get<Size>()) {<br>
cerr << "Integer object parse as Size" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -237,27 +236,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (uint32Obj.get<int32_t>(-1, &ok) != 100 || !ok) {<br>
+ if (uint32Obj.get<int32_t>().value_or(0) != 100) {<br>
cerr << "Unsigned integer object fail to parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (uint32Obj.get<string>("", &ok) != "100" || !ok) {<br>
+ if (uint32Obj.get<string>().value_or("") != "100") {<br>
cerr << "Unsigned integer object fail to parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (uint32Obj.get<double>(1.0, &ok) != 100.0 || !ok) {<br>
+ if (uint32Obj.get<double>().value_or(0.0) != 100.0) {<br>
cerr << "Unsigned integer object fail to parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (uint32Obj.get<uint32_t>(100, &ok) != 100 || !ok) {<br>
+ if (uint32Obj.get<uint32_t>().value_or(0) != 100) {<br>
cerr << "Unsigned integer object parsed as wrong value" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (uint32Obj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (uint32Obj.get<Size>()) {<br>
cerr << "Unsigned integer object parsed as Size" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -275,27 +274,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (doubleObj.get<string>("", &ok) != "3.14159" || !ok) {<br>
+ if (doubleObj.get<string>().value_or("") != "3.14159") {<br>
cerr << "Double object fail to parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (doubleObj.get<double>(1.0, &ok) != 3.14159 || !ok) {<br>
+ if (doubleObj.get<double>().value_or(0.0) != 3.14159) {<br>
cerr << "Double object parse as wrong value" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (doubleObj.get<int32_t>(-1, &ok) != -1 || ok) {<br>
+ if (doubleObj.get<int32_t>()) {<br>
cerr << "Double object parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (doubleObj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (doubleObj.get<uint32_t>()) {<br>
cerr << "Double object parse as unsigned integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (doubleObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (doubleObj.get<Size>()) {<br>
cerr << "Double object parse as Size" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -313,27 +312,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (sizeObj.get<string>("", &ok) != "" || ok) {<br>
+ if (sizeObj.get<string>()) {<br>
cerr << "Size object parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (sizeObj.get<double>(1.0, &ok) != 1.0 || ok) {<br>
+ if (sizeObj.get<double>()) {<br>
cerr << "Size object parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (sizeObj.get<int32_t>(-1, &ok) != -1 || ok) {<br>
+ if (sizeObj.get<int32_t>()) {<br>
cerr << "Size object parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (sizeObj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (sizeObj.get<uint32_t>()) {<br>
cerr << "Size object parse as unsigned integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (sizeObj.get<Size>(Size(0, 0), &ok) != Size(1920, 1080) || !ok) {<br>
+ if (sizeObj.get<Size>().value_or(Size(0, 0)) != Size(1920, 1080)) {<br>
cerr << "Size object parse as wrong value" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -351,27 +350,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (listObj.get<string>("", &ok) != "" || ok) {<br>
+ if (listObj.get<string>()) {<br>
cerr << "List object parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (listObj.get<double>(1.0, &ok) != 1.0 || ok) {<br>
+ if (listObj.get<double>()) {<br>
cerr << "List object parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (listObj.get<int32_t>(-1, &ok) != -1 || ok) {<br>
+ if (listObj.get<int32_t>()) {<br>
cerr << "List object parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (listObj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (listObj.get<uint32_t>()) {<br>
cerr << "List object parse as unsigne integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (listObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (listObj.get<Size>()) {<br>
cerr << "String list object parse as Size" << std::endl;<br>
return TestFail;<br>
}<br>
@@ -424,27 +423,27 @@ protected:<br>
return TestFail;<br>
}<br>
<br>
- if (dictObj.get<string>("", &ok) != "" || ok) {<br>
+ if (dictObj.get<string>()) {<br>
cerr << "Dictionary object parse as string" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (dictObj.get<double>(1.0, &ok) != 1.0 || ok) {<br>
+ if (dictObj.get<double>()) {<br>
cerr << "Dictionary object parse as double" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (dictObj.get<int32_t>(-1, &ok) != -1 || ok) {<br>
+ if (dictObj.get<int32_t>()) {<br>
cerr << "Dictionary object parse as integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (dictObj.get<uint32_t>(1, &ok) != 1 || ok) {<br>
+ if (dictObj.get<uint32_t>()) {<br>
cerr << "Dictionary object parse as unsigned integer" << std::endl;<br>
return TestFail;<br>
}<br>
<br>
- if (dictObj.get<Size>(Size(0, 0), &ok) != Size(0, 0) || ok) {<br>
+ if (dictObj.get<Size>()) {<br>
cerr << "Dictionary object parse as Size" << std::endl;<br>
return TestFail;<br>
}<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
<br>
</blockquote></div></div>