[libcamera-devel] [PATCH v3 1/2] libcamera: Introduce YamlParser as a helper to parse yaml files
Hanlin Chen
hanlinchen at chromium.org
Wed Apr 20 15:37:59 CEST 2022
Hi Laurent,
Thanks for all the comments.
On Tue, Apr 19, 2022 at 11:44 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Apr 18, 2022 at 08:09:22PM +0800, Han-Lin Chen via libcamera-devel wrote:
> > Introduce YamlParser as a helper to convert contents of a yaml file to
> > a tree based structure for easier reading, and to avoid writing parser
> > with raw yaml tokens. The class is based on libyaml, and only support
> > reading but writing a yaml file.
> >
> > The interface is inspired by Json::Value class from jsoncpp:
> > http://jsoncpp.sourceforge.net/class_json_1_1_value.html
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > ---
> > README.rst | 4 +-
> > include/libcamera/internal/meson.build | 1 +
> > include/libcamera/internal/yaml_parser.h | 86 +++
> > src/libcamera/meson.build | 3 +
> > src/libcamera/yaml_parser.cpp | 802 +++++++++++++++++++++++
> > 5 files changed, 894 insertions(+), 2 deletions(-)
> > create mode 100644 include/libcamera/internal/yaml_parser.h
> > create mode 100644 src/libcamera/yaml_parser.cpp
> >
> > diff --git a/README.rst b/README.rst
> > index aae6b79f..b5a2d448 100644
> > --- a/README.rst
> > +++ b/README.rst
> > @@ -60,7 +60,7 @@ Meson Build system: [required]
> > pip3 install --user --upgrade meson
> >
> > for the libcamera core: [required]
> > - python3-yaml python3-ply python3-jinja2
> > + libyaml-dev python3-yaml python3-ply python3-jinja2
> >
> > for IPA module signing: [required]
> > libgnutls28-dev openssl
> > @@ -98,7 +98,7 @@ for tracing with lttng: [optional]
> > liblttng-ust-dev python3-jinja2 lttng-tools
> >
> > for android: [optional]
> > - libexif-dev libjpeg-dev libyaml-dev
> > + libexif-dev libjpeg-dev
> >
> > for lc-compliance: [optional]
> > libevent-dev
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index c9e055d4..7a780d48 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -42,4 +42,5 @@ libcamera_internal_headers = files([
> > 'v4l2_pixelformat.h',
> > 'v4l2_subdevice.h',
> > 'v4l2_videodevice.h',
> > + 'yaml_parser.h',
> > ])
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > new file mode 100644
> > index 00000000..e00ac25d
> > --- /dev/null
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -0,0 +1,86 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * yaml_parser.h - libcamera YAML parsing helper
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
>
> You need to include stdio.h for FILE, or cstdio and use std::FILE.
>
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <libcamera/base/class.h>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +class YamlParserContext;
> > +
> > +class YamlObject
> > +{
> > +public:
> > + YamlObject();
> > + ~YamlObject();
> > +
> > + bool isValue() const
> > + {
> > + return type_ == Value;
> > + }
> > + bool isList() const
> > + {
> > + return type_ == List;
> > + }
> > + bool isDictionary() const
> > + {
> > + return type_ == Dictionary;
> > + }
> > +
> > +#ifndef __DOXYGEN__
> > + template<typename T,
> > + typename std::enable_if_t<
> > + std::is_same<bool, T>::value ||
> > + std::is_same<double, T>::value ||
> > + std::is_same<int32_t, T>::value ||
> > + std::is_same<uint32_t, T>::value ||
> > + std::is_same<std::string, T>::value ||
> > + std::is_same<Size, T>::value> * = nullptr>
> > +#else
> > + template<typename T>
> > +#endif
> > + T get(const T &defaultValue, bool *ok = nullptr) const;
> > +
> > + std::size_t size() const;
> > + const YamlObject &operator[](std::size_t index) const;
> > +
> > + bool contains(const std::string &key) const;
> > + const YamlObject &operator[](const std::string &key) const;
> > + std::vector<std::string> getMemberNames() const;
> > +
> > +private:
> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(YamlObject)
> > +
> > + friend class YamlParserContext;
> > +
> > + enum Type {
> > + Dictionary,
> > + List,
> > + Value,
> > + };
> > +
> > + Type type_;
> > +
> > + std::string value_;
> > + std::vector<std::unique_ptr<YamlObject>> list_;
> > + std::map<const std::string, std::unique_ptr<YamlObject>> dictionary_;
> > +};
> > +
> > +class YamlParser final
> > +{
> > +public:
> > + static std::unique_ptr<YamlObject> parse(FILE *fh);
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 26912ca1..f8e18e03 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -46,6 +46,7 @@ libcamera_sources = files([
> > 'v4l2_pixelformat.cpp',
> > 'v4l2_subdevice.cpp',
> > 'v4l2_videodevice.cpp',
> > + 'yaml_parser.cpp',
> > ])
> >
> > libcamera_sources += libcamera_public_headers
> > @@ -66,6 +67,7 @@ subdir('proxy')
> > libdl = cc.find_library('dl')
> > libgnutls = cc.find_library('gnutls', required : true)
> > libudev = dependency('libudev', required : false)
> > +libyaml = dependency('yaml-0.1', required : true)
> >
> > if libgnutls.found()
> > config_h.set('HAVE_GNUTLS', 1)
> > @@ -126,6 +128,7 @@ libcamera_deps = [
> > libgnutls,
> > liblttng,
> > libudev,
> > + libyaml,
> > ]
> >
> > # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > new file mode 100644
> > index 00000000..f4072bb0
> > --- /dev/null
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -0,0 +1,802 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * yaml_parser.cpp - libcamera YAML parsing helper
> > + */
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include <assert.h>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <yaml.h>
> > +
> > +/**
> > + * \file libcamera/internal/yaml_parser.h
> > + * \brief A YAML parser helper
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(YamlParser)
> > +
> > +namespace {
> > +
> > +void setOk(bool *ok, bool result)
> > +{
> > + if (ok)
> > + *ok = result;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class YamlObject
> > + * \brief A class representing the tree structure of the YAML content
> > + *
> > + * The YamlObject class represents the tree structure of a YAML content. A
>
> s/a YAML content/YAML content/
>
> > + * YamlObject can be a dictionary or list of YamlObjects or a value if a tree
> > + * leaf.
> > + */
> > +YamlObject::YamlObject() = default;
> > +
> > +/**
> > + * \class YamlObject
> > + * \brief Destructor of YamlObject.
>
> s/.$//
>
> > + */
> > +YamlObject::~YamlObject() = default;
> > +
> > +/**
> > + * \fn YamlObject::isValue()
> > + * \brief Return whether the YamlObject is a value
> > + *
> > + * \return True if the YamlObject is a value, false otherwise
> > + */
> > +
> > +/**
> > + * \fn YamlObject::isList()
> > + * \brief Return whether the YamlObject is a list
> > + *
> > + * \return True if the YamlObject is a list, false otherwise
> > + */
> > +
> > +/**
> > + * \fn YamlObject::isDictionary()
> > + * \brief Return whether the YamlObject is a dictionary
> > + *
> > + * \return True if the YamlObject is a dictionary, false otherwise
> > + */
> > +
> > +/**
> > + * \fn template<typename T> YamlObject::get<T>(
> > + * const T &defaultValue, bool *ok) 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 boolean value is returned, and \a ok is set
>
> s/boolean //
>
> > + * to true.
> > + *
> > + * 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
> > + */
> > +
> > +#ifndef __DOXYGEN__
> > +
> > +template<>
> > +bool YamlObject::get(const bool &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != Value)
> > + return defaultValue;
> > +
> > + if (value_ == "true") {
> > + setOk(ok, true);
> > + return true;
> > + } else if (value_ == "false") {
> > + setOk(ok, true);
> > + return false;
> > + }
> > +
> > + return defaultValue;
> > +}
> > +
> > +template<>
> > +int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != Value)
> > + return defaultValue;
> > +
> > + if (value_ == "")
> > + return defaultValue;
> > +
> > + char *end;
> > +
> > + errno = 0;
> > + int32_t value = std::strtol(value_.c_str(), &end, 10);
> > +
> > + if ('\0' != *end || errno == ERANGE)
> > + return defaultValue;
> > +
> > + setOk(ok, true);
> > + return value;
> > +}
> > +
> > +template<>
> > +uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != Value)
> > + return defaultValue;
> > +
> > + if (value_ == "")
> > + return defaultValue;
> > +
> > + char *end;
> > +
> > + errno = 0;
> > + uint32_t value = std::strtoul(value_.c_str(), &end, 10);
> > +
> > + if ('\0' != *end || errno == ERANGE)
> > + return defaultValue;
> > +
> > + setOk(ok, true);
> > + return value;
> > +}
> > +
> > +template<>
> > +double YamlObject::get(const double &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != Value)
> > + return defaultValue;
> > +
> > + if (value_ == "")
> > + return defaultValue;
> > +
> > + char *end;
> > +
> > + errno = 0;
> > + double value = std::strtod(value_.c_str(), &end);
> > +
> > + if ('\0' != *end || errno == ERANGE)
> > + return defaultValue;
> > +
> > + setOk(ok, true);
> > + return value;
> > +}
> > +
> > +template<>
> > +std::string YamlObject::get(const std::string &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != Value)
> > + return defaultValue;
> > +
> > + setOk(ok, true);
> > + return value_;
> > +}
> > +
> > +template<>
> > +Size YamlObject::get(const Size &defaultValue, bool *ok) const
> > +{
> > + setOk(ok, false);
> > +
> > + if (type_ != List)
> > + return defaultValue;
> > +
> > + if (list_.size() != 2)
> > + return defaultValue;
> > +
> > + /*
> > + * Add a local variable to validate of each diemension in case
>
> s/diemension/dimension/
>
> > + * that ok == nullptr.
> > + */
> > + bool valid;
> > + uint32_t width = list_[0]->get<uint32_t>(0, &valid);
> > + if (!valid)
> > + return defaultValue;
> > +
> > + uint32_t height = list_[1]->get<uint32_t>(0, &valid);
> > + if (!valid)
> > + return defaultValue;
> > +
> > + setOk(ok, true);
> > + return Size(width, height);
> > +}
> > +
> > +#endif /* __DOXYGEN__ */
> > +
> > +/**
> > + * \fn YamlObject::size()
> > + * \brief Retrieve the number of elements in a list YamlObject
> > + *
> > + * This function retrieves the size of the YamlObject, defined as the number of
> > + * child elements it contains. Only YamlObject instances of list type have a
> > + * size, calling this function on other types of instances is invalid and
> > + * results in undefined behaviour.
> > + *
> > + * \return The size of the YamlObject
> > + */
> > +std::size_t YamlObject::size() const
> > +{
> > + assert(type_ == List);
> > +
> > + return list_.size();
> > +}
> > +
> > +/**
> > + * \fn YamlObject::operator[](std::size_t index) const
> > + * \brief Retrieve the element from list YamlObject by index
> > + *
> > + * This function retrieves an element of the YamlObject. Only YamlObject
> > + * instances of list type associates elements with index, calling this function
>
> s/associates/associate/
>
> > + * on other types of instances is invalid and results in undefined behaviour.
> > + *
> > + * \return The YamlObject as an element of the list
> > + */
> > +const YamlObject &YamlObject::operator[](std::size_t index) const
> > +{
> > + assert(type_ == List);
> > +
> > + return *list_[index];
> > +}
> > +
> > +/**
> > + * \fn YamlObject::contains()
> > + * \brief Check if an element of a dictionary exists
> > + *
> > + * This function check if the YamlObject contains an element. Only YamlObject
> > + * instances of dictionary type associates elements with names, calling this
>
> s/associates/associate/
>
> > + * function on other types of instances is invalid and results in undefined
> > + * behaviour.
> > + *
> > + * \return True if an element exists, false otherwise
> > + */
> > +bool YamlObject::contains(const std::string &key) const
> > +{
> > + assert(type_ == Dictionary);
> > +
> > + if (dictionary_.find(key) == dictionary_.end())
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * \fn YamlObject::getMemberNames()
> > + * \brief Retrieve all member names of the dictionary
> > + *
> > + * This function retrieve member names of a YamlObject. Only YamlObject
> > + * instances of dictionary type associates elements with names, calling this
>
> s/associates/associate/
>
> > + * function on other types of instances is invalid and results in undefined
> > + * behaviour.
>
> I'd add a comment here with
>
> * \todo Replace this function with an iterator-based API
>
> > + *
> > + * \return A vector of string as the member names
> > + */
> > +std::vector<std::string> YamlObject::getMemberNames() const
> > +{
> > + assert(type_ == Dictionary);
> > +
> > + std::vector<std::string> memberNames;
> > + for (auto &[key, _] : dictionary_)
> > + memberNames.push_back(key);
> > +
> > + return memberNames;
> > +}
> > +
> > +/**
> > + * \fn YamlObject::operator[](const std::string &key) const
> > + * \brief Retrieve a member by name from the dictionary
> > + *
> > + * This function retrieve a memberof a YamlObject by name. Only YamlObject
>
> s/memberof/member of/
>
> > + * instances of dictionary type associates elements with names, calling this
>
> s/associates/associate/
>
> > + * function on other types of instances is invalid and results in undefined
> > + * behaviour.
> > + *
> > + * \return A YamlObject as a member
>
> * \return The YamlObject corresponding to the \a key member
>
> > + */
> > +const YamlObject &YamlObject::operator[](const std::string &key) const
> > +{
> > + assert(type_ == Dictionary);
> > + assert(contains(key));
> > +
> > + auto iter = dictionary_.find(key);
> > + return *iter->second;
> > +}
> > +
> > +class YamlParserContext
> > +{
> > +public:
> > + YamlParserContext();
> > + ~YamlParserContext();
> > +
> > + using ItemParser = const std::function<int()> &;
> > + using RecordParser = const std::function<int(const std::string &)> &;
> > +
> > + int initParser(FILE *fh);
> > + void release();
> > +
> > + int consumeDocumentStart();
> > + int consumeDocumentEnd();
> > +
> > + int parseString(std::string &value);
> > + int parseList(ItemParser parseItem);
> > + int parseDictionary(RecordParser parseRecord);
> > +
> > + int parseNextYamlObject(YamlObject &yamlObject);
> > +
> > +private:
> > + int nextEvent(yaml_event_t &event);
> > + int consume(yaml_event_type_t);
> > +
> > + int parseDictionaryOrList(bool isDictionary,
> > + const std::function<int()> &parseItem);
> > +
> > + bool nextEventLoaded_;
> > + yaml_event_t nextEvent_;
> > +
> > + bool parserValid_;
> > + yaml_parser_t parser_;
> > +};
> > +
> > +/**
> > + * \class YamlParserContext
>
> As this is an internal class, could you exclude it from the doxygen
> generated content ?
>
> > + * \brief Class for YamlParser parsing and context data
> > + *
> > + * The YamlParserContext class stores the internal yaml_parser_t and provides
> > + * helper functions to do event-based parsing for YAML files.
> > + */
> > +YamlParserContext::YamlParserContext()
> > + : nextEventLoaded_(false), parserValid_(false)
> > +{
> > +}
> > +
> > +/**
> > + * \class YamlParserContext
> > + * \brief Destructor of YamlParserContext.
>
> s/.*//
>
> > + */
> > +YamlParserContext::~YamlParserContext()
> > +{
> > + release();
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::initParser()
> > + * \brief Initialize a parser with an opened file for parsing
>
> As this function initializes the whole context, I'd call it init(), and
> document it as
>
> * \brief Initialize the parser context with an open file
>
> > + * \param[in] fh The YAML file to parse
> > + *
> > + * Prior to parsing the YAML content, the YamlParser must be initialized with
>
> s/YamlParser/YamlParserContext/
>
> > + * an opened FILE to create an internal parser. The FILE need to stay valid
>
> s/need/needs/
>
> > + * during the process. The release() must be called after use.
>
> s/release()/release() function/
>
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to initialize
>
> s/is failed/has failed/
>
> > + */
> > +int YamlParserContext::initParser(FILE *fh)
> > +{
> > + /* yaml_parser_initialize returns 1 when succeeded */
>
> s/when succeeded/on success/ (or "when it succeeds")
>
> > + if (!yaml_parser_initialize(&parser_)) {
> > + LOG(YamlParser, Error) << "Failed to initialize YAML parser";
> > + return -EINVAL;
> > + }
> > + parserValid_ = true;
> > + yaml_parser_set_input_file(&parser_, fh);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::release()
> > + * \brief Release the internal parser
> > + *
> > + * After parsing the YAML content, The YamlParser::release() should be called
> > + * to release the internal parser.
> > + */
> > +void YamlParserContext::release()
>
> As the parser context isn't reused, I think you can move the contents of
> this function to the destructor. The comment above that references the
> release() function can be dropped, and that will simplify
> YamlParser::parse().
>
> > +{
> > + if (nextEventLoaded_) {
> > + yaml_event_delete(&nextEvent_);
> > + nextEventLoaded_ = false;
> > + }
> > +
> > + if (parserValid_) {
> > + parserValid_ = false;
> > + yaml_parser_delete(&parser_);
>
> Swap the two lines to match nextEventLoaded_ ?
>
> > + }
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::consume()
> > + * \brief Consume the next event with an expected event type
> > + *
>
> No need for a blank line here.
>
> > + * \param[in] type The expected event type to consume
> > + *
> > + * Consume next event and check whether next event has an expected type.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser failed to initialize
>
> I think this should be
>
> * \retval -EINVAL The \a type doesn't match the next event
>
> > + */
> > +int YamlParserContext::consume(yaml_event_type_t type)
> > +{
> > + yaml_event_t event;
> > + int ret = nextEvent(event);
> > + if (ret)
> > + return ret;
> > +
> > + if (event.type != type) {
> > + LOG(YamlParser, Error)
> > + << "Expect event: " << type
> > + << " but get " << event.type
> > + << " at line: " << event.start_mark.line
> > + << " column: " << event.start_mark.column;
> > + return -EINVAL;
> > + }
> > +
> > + yaml_event_delete(&event);
> > + nextEventLoaded_ = false;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \typedef YamlParserContext::ItemParser
> > + * \brief The functor to handle an item in a YAML list
> > + */
> > +
> > +/**
> > + * \typedef YamlParserContext::RecordParser
> > + * \brief The functor to handle an item in a YAML dictionary
> > + */
> > +
> > +/**
> > + * \fn YamlParserContext::consumeDocumentStart()
> > + * \brief Consume start of a YAML document
> > + *
> > + * Check YAML start of a YAML document. The function should be called and
> > + * checked before parsing any content of the YAML file.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to validate start of a YAML file
> > + */
> > +int YamlParserContext::consumeDocumentStart()
> > +{
> > + if (consume(YAML_STREAM_START_EVENT))
> > + return -EINVAL;
> > +
> > + if (consume(YAML_DOCUMENT_START_EVENT))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::consumeDocumentEnd()
> > + * \brief Consume end of a YAML document
> > + *
> > + * Check YAML end of a YAML document. The function should be called and
> > + * checked after parsing any content of the YAML file.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to validate end of a YAML file
> > + */
> > +int YamlParserContext::consumeDocumentEnd()
> > +{
> > + if (consume(YAML_DOCUMENT_END_EVENT))
> > + return -EINVAL;
> > +
> > + if (consume(YAML_STREAM_END_EVENT))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::parseString()
> > + * \brief Parse scalar and read its content as a string
> > + * \param[in] value The string reference to fill value
> > + *
> > + * A helper function to peek the next event, check whether it's scalar type
> > + * and read its content as a string.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to initialize
> > + */
> > +int YamlParserContext::parseString(std::string &value)
> > +{
> > + yaml_event_t event;
> > + int ret = nextEvent(event);
> > + if (ret)
> > + return ret;
> > +
> > + if (event.type != YAML_SCALAR_EVENT) {
> > + LOG(YamlParser, Error) << "Expect scalar at line: "
> > + << event.start_mark.line
> > + << " column: "
> > + << event.start_mark.column;
> > + return -EINVAL;
> > + }
> > +
> > + value.assign(reinterpret_cast<char *>(event.data.scalar.value),
> > + event.data.scalar.length);
> > +
> > + consume(YAML_SCALAR_EVENT);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::parseList()
> > + * \brief Parse a list with an callback function to parse each item in the list
> > + * \param[in] parseItem The functor to parse a single item in the list
> > + *
> > + * Start to parse a list from the current position and call back to parseItem
> > + * for parsing each item in the list. The parseItem should return 0 on success
> > + * or a negative error code otherwise.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to parse the list
> > + */
> > +int YamlParserContext::parseList(ItemParser parseItem)
> > +{
> > + return parseDictionaryOrList(false, parseItem);
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::parseDictionary()
> > + * \brief Parse a dictionary with an callback function to parse each item
> > + * \param[in] parseRecord The functor to parse a single item in the dictionary
> > + *
> > + * Start to parse a dictionary from the current position and call back to
> > + * parseRecord to parse value by a string argument as the key to the value.
> > + * The parseRecord should return 0 on success or a negative error code
> > + * otherwise.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to parse the dictionary
> > + */
> > +int YamlParserContext::parseDictionary(RecordParser parseRecord)
> > +{
> > + auto parseItem = [this, &parseRecord]() {
> > + std::string key;
> > + int ret = parseString(key);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + return parseRecord(key);
> > + };
> > +
> > + return parseDictionaryOrList(true, parseItem);
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::parseDictionaryOrList()
> > + * \brief A helper function to abstract common part of parsing dictionary or list
> > + *
> > + * \param[in] isDictionary True for parsing a dictionary, and false for a list
> > + * \param[in] parseItem The callback to handle an item
> > + *
> > + * A helper function to abstract parsing a item from a dictionary or a list.
> > + * The differences of them in a YAML event stream are:
> > + *
> > + * 1. The start and end event type are different
> > + * 2. There is a leading scalar string as key in the items of a dictionary
> > + *
> > + * The caller should handle the leading key string in its callback parseItem
> > + * when it's a dictionary.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to initialize
> > + */
> > +int YamlParserContext::parseDictionaryOrList(bool isDictionary,
> > + const std::function<int()> &parseItem)
> > +{
> > + yaml_event_type_t startEventType = YAML_SEQUENCE_START_EVENT;
> > + yaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;
> > +
> > + if (isDictionary) {
> > + startEventType = YAML_MAPPING_START_EVENT;
> > + endEventType = YAML_MAPPING_END_EVENT;
> > + }
> > +
> > + if (consume(startEventType))
> > + return -EINVAL;
> > +
> > + /*
> > + * Add a safety counter to make sure we don't loop indefinitely in case
> > + * the configuration file is malformed.
> > + */
> > + unsigned int sentinel = 1000;
> > + yaml_event_t event;
> > + do {
> > + int ret = nextEvent(event);
> > + if (ret)
> > + return ret;
> > +
> > + if (event.type == endEventType)
> > + return consume(endEventType);
> > +
> > + ret = parseItem();
> > + if (ret)
> > + return ret;
> > +
> > + --sentinel;
> > + } while (sentinel);
> > +
> > + if (!sentinel)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::nextEvent()
> > + * \brief Peek the next event
> > + *
> > + * \param[in] event The event reference to fill information
> > + *
> > + * Peek the next event in the current YAML event stream, and return -EINVAL when
> > + * there is no more event.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL The parser is failed to initialize
> > + */
> > +int YamlParserContext::nextEvent(yaml_event_t &event)
> > +{
> > + if (nextEventLoaded_) {
> > + event = nextEvent_;
> > + return 0;
> > + }
>
> I'm not a big fan of this mechanism. Wouldn't it be possible to always
> consume the next event, possibly returning it from this function as a
> unique_ptr, and passing it down the call stack where needed ? For
> instance the event returned by nextEvent() in
> YamlParserContext::parseNextYamlObject() would be passed to
> parseString(). I think it would simplify the code, as some checks could
> also be removed (for instance the event type check in parseString(), as
> the type is checked in parseNextYamlObject() already).
Yes, I was struggled with this...
If it always consumes the next event, the first event has to pass to
parseItem of parseDictionaryOrList(), and thus propagate to all
intermediate handlers and to (by recursion) parseNextYamlObject().
There was an intermediate version which is the case, but I abandoned
it because it seems unclean to me.
nextEvent() is added after that to peek the next event for conditional
check, and let each item handler consume its start and end token
symmetrically.
>
> > +
> > + /* yaml_parser_parse returns when it succeeds */
> > + if (1 != yaml_parser_parse(&parser_, &nextEvent_)) {
> > + return -EINVAL;
> > + }
>
> No need for curly braces.
>
> > +
> > + nextEventLoaded_ = true;
> > + event = nextEvent_;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \fn YamlParserContext::parseNextYamlObject()
> > + * \brief Parse next YAML event and read it as a YamlObject
> > + * \param[in] yamlObject The result of YamlObject
> > + *
> > + * Parse next YAML event by peeking next event and parse
> > + * them separately as a value, list or dictionary.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EINVAL Fail to parse the YAML file.
> > + */
> > +int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject)
> > +{
> > + yaml_event_t event;
> > +
> > + if (nextEvent(event))
> > + return -EINVAL;
> > +
> > + if (event.type == YAML_SCALAR_EVENT) {
>
> It looks like a switch/case would be a good replacement for the if's.
>
> > + yamlObject.type_ = YamlObject::Value;
> > + parseString(yamlObject.value_);
> > + return 0;
> > + }
> > +
> > + if (event.type == YAML_SEQUENCE_START_EVENT) {
> > + yamlObject.type_ = YamlObject::List;
> > + auto &list = yamlObject.list_;
> > + auto handler = [this, &list]() {
> > + list.emplace_back(new YamlObject());
> > + return parseNextYamlObject(*list.back());
> > + };
> > + return parseList(handler);
> > + }
> > +
> > + if (event.type == YAML_MAPPING_START_EVENT) {
> > + yamlObject.type_ = YamlObject::Dictionary;
> > + auto &dictionary = yamlObject.dictionary_;
> > + auto handler = [this, &dictionary](const std::string &key) {
> > + dictionary[key].reset(new YamlObject());
> > + return parseNextYamlObject(*dictionary[key]);
> > + };
> > + return parseDictionary(handler);
> > + }
> > +
> > + LOG(YamlParser, Error) << "Invalid YAML file";
> > + return -EINVAL;
> > +}
> > +
> > +/**
> > + * \class YamlParser
> > + * \brief A helper class for parsing YAML files.
>
> s/files.$/files/
>
> > + *
> > + * The YamlParser class provides an easy interface to parse the contents of a
> > + * YAML file int a tree fo YamlObject instances.
>
> s/tree fo/tree of/
>
> > + *
> > + * Example usage 1:
> > + * The following code illustrates how to parse the following YAML file:
> > + *
> > + * name:
> > + * "John"
> > + * numbers:
> > + * - 1
> > + * - 2
>
> You need to put this in a quote block, or doxygen will likely misrender
> it.
>
> > + *
> > + * \code
> > + *
> > + * YamlObject root;
> > + * std::unique_ptr<YamlObject> root = YamlParser::parse(fh);
> > + * if (!root)
> > + * return;
> > + *
> > + * if (!root->isDictionary())
> > + * return;
> > + *
> > + * std::string name = (*root)["name"];
>
> std::string isn't the right return type.
>
> > + * std::cout << name.get<std::string>("");
> > + *
> > + * const YamlObject &numbers = (*root)["numbers"];
> > + * if (!numbers.isList())
> > + * return;
> > + *
> > + * for (int i = 0; i < numbers.size; i++)
> > + * std::cout << numbers[i].get<int32_t>(0);
>
> for (std::size_t i = 0; i < numbers.size(); i++)
> std::cout << numbers[i].get<int32_t>(0) << std::endl;
>
> It would be nice to try and compile that code snippet.
>
> > + *
> > + * \endcode
> > + *
> > + * The YamlParser::parse() function accepts an open FILE and initializes an
> > + * internal parse.
>
> Let's not document the internal implementation details in the public
> documentation:
>
> * The YamlParser::parse() function takes open FILE, parses its contents, and
> * returns a pointer to a YamlObject corresponding to the root node of the YAML
> * document.
>
> > + */
> > +
> > +/**
> > + * \fn YamlParser::parse()
> > + * \brief Parse a YAML file as a YamlObject
> > + * \param[in] fh The YAML file to parse
> > + *
> > + * Prior to parsing the YAML content, the function accepts an opened FILE to
> > + * create an internal parser.
>
> Same here.
>
> * This function takes open FILE, parses its contents, and returns a pointer to
> * a YamlObject corresponding to the root node of the YAML document. The caller
> * is responsible for closing the file after parsing completes.
>
> > + *
> > + * \return Pointer to result YamlObject on success or nullptr otherwise
> > + */
> > +std::unique_ptr<YamlObject> YamlParser::parse(FILE *fh)
> > +{
> > + YamlParserContext context;
> > +
> > + if (context.initParser(fh))
> > + return nullptr;
> > +
> > + std::unique_ptr<YamlObject> root(new YamlObject());
> > +
> > + if (context.consumeDocumentStart())
> > + goto error;
> > +
> > + if (context.parseNextYamlObject(*root))
> > + goto error;
> > +
> > + if (context.consumeDocumentEnd())
> > + goto error;
> > +
> > + context.release();
> > + return root;
> > +
> > +error:
> > + root.release();
>
> No need for this, the unique_ptr will be destroyed when this function
> returns.
>
> > + context.release();
> > + return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list