| Message ID | 20220803110324.28539-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Commit | 2e77ccbb93a14a0c976bb3ca9d5dbbdc32a12847 |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Aug 03, 2022 at 01:03:24PM +0200, Jacopo Mondi via libcamera-devel wrote: > The YamlParser::getList<>() function returns an std::optional<> to allow > callers to identify cases where parsing the .yaml file failed from cases > where the parsed list is just empty. > > The current implementation however returns an empty list in case of > errors, making it impossible for the caller to detect parsing failures. > > Fix this by returning std::nullopt in case of errors. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/yaml_parser.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp > index 84cb57d6de83..c96e99e1317c 100644 > --- a/src/libcamera/yaml_parser.cpp > +++ b/src/libcamera/yaml_parser.cpp > @@ -319,7 +319,7 @@ template<typename T, > std::optional<std::vector<T>> YamlObject::getList() const > { > if (type_ != Type::List) > - return {}; > + return std::nullopt; > > std::vector<T> values; > values.reserve(list_.size()); > @@ -327,7 +327,7 @@ std::optional<std::vector<T>> YamlObject::getList() const > for (const YamlObject &entry : asList()) { > const auto value = entry.get<T>(); > if (!value) > - return {}; > + return std::nullopt; > values.emplace_back(*value); > } > > -- > 2.37.1 >
Hi Jacopo, Thank you for the patch. On Wed, Aug 03, 2022 at 01:03:24PM +0200, Jacopo Mondi via libcamera-devel wrote: > The YamlParser::getList<>() function returns an std::optional<> to allow > callers to identify cases where parsing the .yaml file failed from cases > where the parsed list is just empty. > > The current implementation however returns an empty list in case of > errors, making it impossible for the caller to detect parsing failures. That's actually not true: -------- #include <iostream> #include <optional> #include <vector> static std::optional<std::vector<int>> foo(bool null) { if (null) return std::nullopt; else return {}; } int main() { std::optional<std::vector<int>> opt; opt = foo(true); std::cout << "- opt " << (opt == std::nullopt ? "==" : "!=") << " std::nullopt" << std::endl; opt = foo(false); std::cout << "- opt " << (opt == std::nullopt ? "==" : "!=") << " std::nullopt" << std::endl; return 0; } -------- prints -------- - opt == std::nullopt - opt == std::nullopt -------- {} means a default-constructed object, whose type is inferred from the return type of the function here, so std::optional<std::vector<int>>() in this case, which is equal to std::nullopt. However, I agree that std::nullopt is more explicit, so I like the change, but the commit message should be reworded. > Fix this by returning std::nullopt in case of errors. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/yaml_parser.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp > index 84cb57d6de83..c96e99e1317c 100644 > --- a/src/libcamera/yaml_parser.cpp > +++ b/src/libcamera/yaml_parser.cpp > @@ -319,7 +319,7 @@ template<typename T, > std::optional<std::vector<T>> YamlObject::getList() const > { > if (type_ != Type::List) > - return {}; > + return std::nullopt; > > std::vector<T> values; > values.reserve(list_.size()); > @@ -327,7 +327,7 @@ std::optional<std::vector<T>> YamlObject::getList() const > for (const YamlObject &entry : asList()) { > const auto value = entry.get<T>(); > if (!value) > - return {}; > + return std::nullopt; > values.emplace_back(*value); > } >
diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp index 84cb57d6de83..c96e99e1317c 100644 --- a/src/libcamera/yaml_parser.cpp +++ b/src/libcamera/yaml_parser.cpp @@ -319,7 +319,7 @@ template<typename T, std::optional<std::vector<T>> YamlObject::getList() const { if (type_ != Type::List) - return {}; + return std::nullopt; std::vector<T> values; values.reserve(list_.size()); @@ -327,7 +327,7 @@ std::optional<std::vector<T>> YamlObject::getList() const for (const YamlObject &entry : asList()) { const auto value = entry.get<T>(); if (!value) - return {}; + return std::nullopt; values.emplace_back(*value); }
The YamlParser::getList<>() function returns an std::optional<> to allow callers to identify cases where parsing the .yaml file failed from cases where the parsed list is just empty. The current implementation however returns an empty list in case of errors, making it impossible for the caller to detect parsing failures. Fix this by returning std::nullopt in case of errors. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/yaml_parser.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)