[1/3] libcamera: Implement YamlEmitter

21819 diff mbox series

Implement a helpers to allow outputting text in YAML format.

The class of helpers allows to create list and dictionaries and emit
scalar values starting from a YamlRoot object initialized with
a file path.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/internal/meson.build    |   1 +
 include/libcamera/internal/yaml_emitter.h | 164 ++++++
 src/libcamera/meson.build                 |   1 +
 src/libcamera/yaml_emitter.cpp            | 577 ++++++++++++++++++++++
 4 files changed, 743 insertions(+)
 create mode 100644 include/libcamera/internal/yaml_emitter.h
 create mode 100644 src/libcamera/yaml_emitter.cpp

Hi Jacopo,

Thank you for the patch.

On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> Implement a helpers to allow outputting text in YAML format.

s/helpers/helper/

> 
> The class of helpers allows to create list and dictionaries and emit

s/list/lists/

> scalar values starting from a YamlRoot object initialized with
> a file path.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/internal/meson.build    |   1 +
>  include/libcamera/internal/yaml_emitter.h | 164 ++++++
>  src/libcamera/meson.build                 |   1 +
>  src/libcamera/yaml_emitter.cpp            | 577 ++++++++++++++++++++++
>  4 files changed, 743 insertions(+)
>  create mode 100644 include/libcamera/internal/yaml_emitter.h
>  create mode 100644 src/libcamera/yaml_emitter.cpp
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1c5eef9cab80..7533b075fde2 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
>      'v4l2_pixelformat.h',
>      'v4l2_subdevice.h',
>      'v4l2_videodevice.h',
> +    'yaml_emitter.h',
>      'yaml_parser.h',
>  ])
>  
> diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> new file mode 100644
> index 000000000000..78196a7f147f
> --- /dev/null
> +++ b/include/libcamera/internal/yaml_emitter.h
> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * libcamera YAML emitter helper
> + */
> +
> +#pragma once
> +
> +#include <memory>
> +#include <string>
> +#include <string_view>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/file.h>
> +
> +#include <yaml.h>
> +
> +namespace libcamera {
> +
> +class YamlDict;
> +class YamlEvent;
> +class YamlList;
> +class YamlRoot;
> +class YamlScalar;
> +
> +class YamlEmitter final
> +{
> +public:
> +	~YamlEmitter();
> +
> +	static YamlRoot root(const std::string &path);
> +
> +private:
> +	friend class YamlOutput;
> +	friend class YamlRoot;
> +
> +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> +
> +	YamlEmitter(const std::string &path);
> +
> +	void logError();
> +	void init();
> +	int emit();
> +
> +	File file_;
> +	yaml_event_t event_;
> +	yaml_emitter_t emitter_;
> +};
> +
> +class YamlOutput
> +{
> +public:
> +	bool valid() const { return !!emitter_; }
> +
> +protected:
> +	YamlOutput() = default;
> +
> +	YamlOutput(YamlEmitter *emitter)
> +		: emitter_(emitter)
> +	{
> +	}
> +
> +	YamlOutput &operator=(YamlOutput &&other)
> +	{
> +		emitter_ = other.emitter_;
> +		other.emitter_ = nullptr;
> +
> +		return *this;
> +	}
> +
> +	int emitScalar(std::string_view scalar);
> +	int emitMappingStart();
> +	int emitMappingEnd();
> +	int emitSequenceStart();
> +	int emitSequenceEnd();
> +
> +	YamlScalar scalar();
> +	YamlDict dict();
> +	YamlList list();
> +
> +	YamlEmitter *emitter_ = nullptr;
> +	yaml_event_t event_;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(YamlOutput)
> +};
> +
> +class YamlRoot : public YamlOutput
> +{
> +public:
> +	YamlRoot() = default;
> +	~YamlRoot();
> +
> +	YamlRoot &operator=(YamlRoot &&other) = default;
> +
> +	YamlList list();
> +	YamlDict dict();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(YamlRoot);
> +
> +	friend class YamlEmitter;
> +
> +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> +	{
> +	}
> +
> +	std::unique_ptr<YamlEmitter> emitterRoot_;
> +};
> +
> +class YamlScalar : public YamlOutput
> +{
> +public:
> +	YamlScalar() = default;
> +	~YamlScalar() = default;
> +
> +	void operator=(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlScalar(YamlEmitter *emitter);
> +};
> +
> +class YamlList : public YamlOutput
> +{
> +public:
> +	YamlList() = default;
> +	~YamlList();
> +
> +	YamlList &operator=(YamlList &&other) = default;
> +
> +	YamlList list();
> +	YamlDict dict();
> +	void scalar(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlList(YamlEmitter *emitter);
> +};
> +
> +class YamlDict : public YamlOutput
> +{
> +public:
> +	YamlDict() = default;
> +	~YamlDict();
> +
> +	YamlDict &operator=(YamlDict &&other) = default;
> +
> +	YamlList list(std::string_view key);
> +	YamlDict dict(std::string_view key);
> +
> +	YamlScalar operator[](std::string_view key);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlDict(YamlEmitter *emitter);
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index aa9ab0291854..5b8af4103085 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
>      'v4l2_pixelformat.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
> +    'yaml_emitter.cpp',
>      'yaml_parser.cpp',
>  ])
>  
> diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> new file mode 100644
> index 000000000000..fa3de91ce988
> --- /dev/null
> +++ b/src/libcamera/yaml_emitter.cpp
> @@ -0,0 +1,577 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * libcamera YAML emitter helper
> + */
> +
> +#include "libcamera/internal/yaml_emitter.h"
> +
> +#include <libcamera/base/log.h>

Missing

#include <libcamera/base/span.h>

You should also include errno.h.

> +
> +/**
> + * \file yaml_emitter.h
> + * \brief A YAML emitter helper
> + *
> + * The YAML emitter helpers allow users to emit output in YAML format.

I would outline here the key difference with a YAML writer.

 * Unlike YAML writers that operate on a fully populated representation of the
 * data, the YAML emitter outputs YAML data on the fly. It is suitable for
 * outputting large amount of data with low overhead and no runtime heap
 * allocation.

> + *
> + * To emit YAML users of the these helper classes create a root node with
> + *
> + * \code
> +	std::string filePath("...");
> +	auto root = YamlEmitter::root(filePath);
> +   \endcode
> + *
> + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> + * and YamlRoot::list() functions.

I would write "and start emitting dictionaries and lists", "populating"
sounds more like a YAML writer.

> + *
> + * The classes part of this file implement RAII-style handling of YAML
> + * events. By creating a YamlList and YamlDict instance the associated YAML
> + * sequence start and mapping start events are emitted and once the instances
> + * gets destroyed the corresponding sequence end and mapping end events are
> + * emitted.
> + *
> + * From an initialized YamlRoot instance is possible to create YAML list and
> + * dictionaries.
> + *
> + * \code
> +	YamlDict dict = root.dict();
> +	YamlList list = root.list();
> +   \endcode
> + *
> + * YamlDict instances can be populated with scalars associated with a key
> + *
> + * \code
> +	dict["key"] = "value";
> +   \endcode
> + *
> + * and it is possible to create lists and dictionaries, associated with a key
> + *
> + * \code
> +	YamlDict subDict = dict.dict("newDict");
> +	YamlList subList = dict.list("newList");
> +   \endcode
> + *
> + * YamlList instances can be populated with scalar elements
> + *
> + * \code
> +	list.scalar("x");
> +	list.scalar("y");
> +   \endcode
> + *
> + * and with dictionaries and lists too
> + *
> + * \code
> +	YamlDict subDict = list.dict();
> +	YamlList subList = list.list();
> +   \endcode

The biggest issue with the API is that it can be easily misused:

	YamlDict list1 = dict.list("foo");
	YamlDict list2 = dict.list("bar");
	list1.scalar("0");

I'm still annoyed that I haven't found a neat way to prevent this. I
wonder if we could make it a bit safer by at least catching incorrect
usage at runtime. What I'm thinking is

- When creating a child, recording the child pointer in the parent, and
  the parent pointer in the child
- If the parent already has a child when a new child is created, reset
  the parent pointer of the previous child. This makes the previous
  child invalid.
- Any invalidation of a child needs to be propagated to its children.
- Any operation on an invalid child would be caught and logged.
- When a child is destroyed, if its parent is not null, reset the child
  pointer in the parent to null.
- If a parent is destroyed while it has a child pointer, reset the
  child's parent pointer to null and log an error.

> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(YamlEmitter)
> +
> +namespace {
> +
> +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> +{
> +	File *file = static_cast<File *>(data);
> +
> +	Span<unsigned char> buf{ buffer, size };
> +	ssize_t ret = file->write(buf);
> +	if (ret < 0) {
> +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);

s/error :/error:/

> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \class YamlEmitter
> + *
> + * YAML helper classes entry point. This class allows to create a YamlRoot
> + * instances, using the YamlEmitter::root() function, that users can populate

s/instances/instance/

> + * with lists, dictionaries and scalars.
> + */
> +
> +YamlEmitter::YamlEmitter(const std::string &path)
> +{
> +	std::string filePath(path);
> +	file_.setFileName(filePath);

I don't think you need the local variable, you can write

	file_.setFileName(path);

> +	file_.open(File::OpenModeFlag::WriteOnly);
> +}
> +
> +/**
> + * \brief Destroy the YamlEmitter
> + */

I think you can drop this, the documentation doesn't bring much, and the
function isn't part of the API exposed to users anyway.

> +YamlEmitter::~YamlEmitter()
> +{
> +	yaml_event_delete(&event_);
> +	yaml_emitter_delete(&emitter_);
> +}
> +
> +/**
> + * \brief Create an initialized instance of YamlRoot
> + * \param[in] path The YAML output file path
> + *
> + * Create an initialized instance of the YamlRoot class that users can start
> + * using and populating with scalers, lists and dictionaries.

s/scalers/scalars/

> + *
> + * \return An initialized YamlRoot instance
> + */
> +YamlRoot YamlEmitter::root(const std::string &path)

I asked in the review of the preview version if we should pass a
std::ostream to make this more generic. I'm fine ignoring it for now, as
it's unclear if we would have use cases for that.

> +{
> +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(path) };
> +
> +	emitter->init();
> +
> +	return YamlRoot(std::move(emitter));
> +}
> +
> +void YamlEmitter::logError()
> +{
> +	switch (emitter_.error) {
> +	case YAML_MEMORY_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Memory error: Not enough memory for emitting";
> +		break;
> +
> +	case YAML_WRITER_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Writer error: " << emitter_.problem;
> +		break;
> +
> +	case YAML_EMITTER_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Emitter error: " << emitter_.problem;
> +		break;
> +
> +	default:
> +		LOG(YamlEmitter, Error) << "Internal problem";

"Internal error" ?

> +		break;
> +	}
> +}
> +
> +void YamlEmitter::init()
> +{
> +	yaml_emitter_initialize(&emitter_);
> +	yaml_emitter_set_output(&emitter_, yamlWrite, &file_);
> +
> +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> +	emit();
> +
> +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);

nullptr ? Same elsewhere.

> +	emit();
> +}
> +
> +int YamlEmitter::emit()
> +{
> +	int ret = yaml_emitter_emit(&emitter_, &event_);
> +	if (!ret) {
> +		logError();
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \class YamlOutput
> + *
> + * The YamlOutput base class. From this class are derived the YamlScalar,
> + * YamlList and YamlDict classes which are meant to be used by users of the
> + * YAML emitter helpers.
> + *
> + * The YamlOutput base class provides functions to create YAML lists and
> + * dictionaries and to populate them.
> + *
> + * Instances of this class cannot be instantiated directly by applications.

s/Instances of this/This/
s/ by applications// as none of this is exposed to applications :-)

> + */
> +
> +/**
> + * \fn YamlOutput::valid()
> + * \brief Check if a YamlOutput instance has been correctly initialized
> + * \return True if the instance has been initialized, false otherwise
> + */

We usually document constructors first.

> +
> +/**
> + * \fn YamlOutput::YamlOutput(YamlEmitter *emitter)
> + * \brief Create a YamlOutput instance with an associated emitter
> + * \param[in] emitter The YAML emitter
> + */
> +
> +/**
> + * \fn YamlOutput &YamlOutput::operator=(YamlOutput &&other)
> + * \brief The move-assignment operator
> + * \param[in] other The instance to be moved
> + */
> +
> +/**
> + * \brief Emit \a scalar as a YAML scalar
> + * \param[in] scalar The element to emit
> + * \return 0 in case of success, a negative error value otherwise
> + */
> +int YamlOutput::emitScalar(std::string_view scalar)
> +{
> +	if (!valid())
> +		return -EINVAL;
> +
> +	const yaml_char_t *value = reinterpret_cast<const yaml_char_t *>
> +				   (scalar.data());
> +	yaml_scalar_event_initialize(&emitter_->event_, NULL, NULL, value,
> +				     scalar.length(), true, false,
> +				     YAML_PLAIN_SCALAR_STYLE);
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \brief Emit the mapping start YAML event
> + * \return 0 in case of success, a negative error value otherwise
> + */
> +int YamlOutput::emitMappingStart()
> +{
> +	if (!valid())
> +		return -EINVAL;
> +
> +	yaml_mapping_start_event_initialize(&emitter_->event_, NULL, NULL,
> +					    true, YAML_BLOCK_MAPPING_STYLE);
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \brief Emit the mapping end YAML event
> + * \return 0 in case of success, a negative error value otherwise
> + */
> +int YamlOutput::emitMappingEnd()
> +{
> +	if (!valid())
> +		return -EINVAL;
> +
> +	yaml_mapping_end_event_initialize(&emitter_->event_);
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \brief Emit the sequence start YAML event
> + * \return 0 in case of success, a negative error value otherwise
> + */
> +int YamlOutput::emitSequenceStart()
> +{
> +	if (!valid())
> +		return -EINVAL;
> +
> +	yaml_sequence_start_event_initialize(&emitter_->event_, NULL, NULL,
> +					     true, YAML_BLOCK_SEQUENCE_STYLE);
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \brief Emit the sequence end YAML event
> + * \return 0 in case of success, a negative error value otherwise
> + */
> +int YamlOutput::emitSequenceEnd()
> +{
> +	if (!valid())
> +		return -EINVAL;
> +
> +	yaml_sequence_end_event_initialize(&emitter_->event_);
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \brief Create a scalar instance
> + * \return An instance of YamlScalar
> + */
> +YamlScalar YamlOutput::scalar()
> +{
> +	return YamlScalar(emitter_);
> +}
> +
> +/**
> + * \brief Create a dictionary instance
> + * \return An instance of YamlDict
> + */
> +YamlDict YamlOutput::dict()
> +{
> +	return YamlDict(emitter_);
> +}
> +
> +/**
> + * \brief Create a list instance
> + * \return An instance of YamlList
> + */
> +YamlList YamlOutput::list()
> +{
> +	return YamlList(emitter_);
> +}
> +
> +/**
> + * \var YamlOutput::emitter_
> + * \brief The emitter used by this YamlObject to output YAML events
> + */
> +
> +/**
> + * \var YamlOutput::event_
> + * \brief The YAML event used by this YamlObject
> + */
> +
> +/**
> + * \class YamlRoot
> + *
> + * The YAML root node. A valid YamlRoot instance can only be created using the
> + * YamlEmitter::root() function. The typical initialization pattern of users of
> + * this class is similar to the one in the following example:
> + *
> + * \code
> +	class YamlUser
> +	{
> +	public:
> +		YamlUser();
> +
> +	private:
> +		YamlRool root_;
> +	};
> +
> +	YamlUser::YamlUser()
> +	{
> +		root_ = YamlEmitter::root("/path/to/yaml/file.yml");
> +	}
> +   \endcode
> + *
> + * A YamlRoot element can be populated with list and dictionaries.
> + */
> +
> +/**
> + * \fn YamlRoot::YamlRoot()
> + * \brief Construct a YamlRoot instance without initializing it
> + *
> + * A YamlRoot instance can be created in non-initialized state typically to be
> + * stored as a class member by the users of this class. In order to start using
> + * and populating the YamlRoot instance a valid and initialized instance,
> + * created using the YamlEmitter::root() function, has to be move-assigned to
> + * the non-initialized instance.
> + *
> + * \code
> +	YamlRoot root;
> +
> +	root = YamlEmitter::root("/path/to/yaml/file.yml");
> +   \endcode
> + */
> +
> +/**
> + * \brief Delete a YamlRoot
> + */
> +YamlRoot::~YamlRoot()
> +{
> +	if (!valid())
> +		return;
> +
> +	yaml_document_end_event_initialize(&emitter_->event_, 0);
> +	emitterRoot_->emit();
> +
> +	yaml_stream_end_event_initialize(&emitter_->event_);
> +	emitterRoot_->emit();
> +}
> +
> +/**
> + * \fn YamlRoot &YamlRoot::operator=(YamlRoot &&other)
> + * \brief Move assignment operator
> + *
> + * Move-assign a YamlRoot instance. This function is typically used to assign an
> + * initialized instance returned by YamlEmitter::root() to a non-initialized
> + * one.
> + *
> + * \return A reference to this instance of YamlRoot
> + */
> +
> +/**
> + * \copydoc YamlOutput::dict()
> + */
> +YamlDict YamlRoot::dict()
> +{
> +	int ret = emitMappingStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::dict();
> +}
> +
> +/**
> + * \copydoc YamlOutput::list()
> + */
> +YamlList YamlRoot::list()
> +{
> +	int ret = emitSequenceStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::list();
> +}
> +
> +/**
> + * \class YamlScalar
> + *
> + * A YamlScalar can be assigned to an std::string_view to emit them as YAML
> + * elements.
> + */
> +
> +/**
> + * \brief Create a YamlScalar instance
> + */
> +YamlScalar::YamlScalar(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +/**
> + * \brief Emit \a scalar as a YAML scalar
> + * \param[in] scalar The element to emit in the YAML output
> + */
> +void YamlScalar::operator=(std::string_view scalar)
> +{
> +	emitScalar(scalar);
> +}
> +
> +/**
> + * \class YamlList
> + *
> + * A YamlList can be populated with scalars and allows to create nested lists
> + * and dictionaries.
> + */
> +
> +/**
> + * \brief Create a YamlList
> + */
> +YamlList::YamlList(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +/**
> + * \brief Destroy a YamlList instance
> + */
> +YamlList::~YamlList()
> +{
> +	emitSequenceEnd();
> +}
> +
> +/**
> + * \fn YamlList &YamlList::operator=(YamlList &&other)
> + * \brief Move-assignment operator
> + * \param[inout] other The instance to move
> + */
> +
> +/**
> + * \brief Append \a scalar to the list
> + * \param[in] scalar The element to append to the list
> + */
> +void YamlList::scalar(std::string_view scalar)
> +{
> +	emitScalar(scalar);
> +}
> +
> +/**
> + * \copydoc YamlOutput::list()
> + */
> +YamlList YamlList::list()
> +{
> +	int ret = emitSequenceStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::list();
> +}
> +
> +/**
> + * \copydoc YamlOutput::dict()
> + */
> +YamlDict YamlList::dict()
> +{
> +	int ret = emitMappingStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::dict();
> +}
> +
> +/**
> + * \class YamlDict
> + *
> + * A YamlDict can be populated with scalars using operator[] and allows to
> + * create other lists and dictionaries associated with a key.
> + */
> +
> +/**
> + * \fn YamlDict::YamlDict()
> + * \brief Create a non-initialized instance of a YamlDict
> + */
> +
> +YamlDict::YamlDict(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +/**
> + * \brief Destroy a YamlDict instance
> + */
> +YamlDict::~YamlDict()
> +{
> +	emitMappingEnd();
> +}
> +
> +/**
> + * \fn YamlDict &YamlDict::operator=(YamlDict &&other)
> + * \brief Move-assignment operator
> + * \param[inout] other The instance to move

I think we usually use param[in] for move constructors and assignment
operators. Granted, it's a bit borderline.

> + */
> +
> +/**
> + * \copydoc YamlOutput::list()

That won't document the key parameter.

> + */
> +YamlList YamlDict::list(std::string_view key)
> +{
> +	int ret = emitScalar(key);
> +	if (ret)
> +		return {};
> +
> +	ret = emitSequenceStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::list();
> +}
> +
> +/**
> + * \copydoc YamlOutput::dict()

Same here.

> + */
> +YamlDict YamlDict::dict(std::string_view key)
> +{
> +	int ret = emitScalar(key);
> +	if (ret)
> +		return {};
> +
> +	ret = emitMappingStart();
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::dict();
> +}
> +
> +/**
> + * \brief Create a scalar associated with \a key in the dictionary
> + * \param[in] key The key associated with the newly created scalar
> + * \return A YamlScalar that application can use to output text
> + */
> +YamlScalar YamlDict::operator[](std::string_view key)

I'm still not entirely thrilled by this, for a few reasons. None are
showstoppers, you can decide what to do.

First there's an asymmetry in the API. To emit scalars on lists you have
a scalar() function, while here you use the [] operator. Similarly, in
the YamlDict class, you have explicitly named functions to emit a list
or dictionary, but not for scalars. Now asymmetries are not always bad,
sometimes specific APIs are best for specific jobs.

Another API design point that bothers me possibly a bit more is the
difference in behaviour between YamlDict::operator[]() and
std::map::operator[](). Using the [] operators, users could expect that

	dict["foo"] = "bar";
	dict["foo"] = "baz";

will result in the second statement overwriting the first one. The
YamlDict::operator[]() API is a bit misleading. As it's an internal
class it's less of an issue than it would be for the libcamera public
API, but still.

Finally, returning a YamlScalar object opens the door for misuse. The
following code will compile, but is invalid:

	YamlScalar scalar = dict["foo"];
	dict["bar"] = "0";
	scalar = "1";

Using an explicit function would solve all this:

	dict.scalar("foo", "0");

The YamlScalar class could then be moved from the .h file to the .cpp
file, and possibly even dropped completely.

> +{
> +	int ret = emitScalar(key);
> +	if (ret)
> +		return {};
> +
> +	return YamlOutput::scalar();
> +}
> +
> +} /* namespace libcamera */

Hi Laurent thanks for review

On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > Implement a helpers to allow outputting text in YAML format.
>
> s/helpers/helper/
>
> >
> > The class of helpers allows to create list and dictionaries and emit
>
> s/list/lists/
>
> > scalar values starting from a YamlRoot object initialized with
> > a file path.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  include/libcamera/internal/meson.build    |   1 +
> >  include/libcamera/internal/yaml_emitter.h | 164 ++++++
> >  src/libcamera/meson.build                 |   1 +
> >  src/libcamera/yaml_emitter.cpp            | 577 ++++++++++++++++++++++
> >  4 files changed, 743 insertions(+)
> >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> >  create mode 100644 src/libcamera/yaml_emitter.cpp
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1c5eef9cab80..7533b075fde2 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
> >      'v4l2_pixelformat.h',
> >      'v4l2_subdevice.h',
> >      'v4l2_videodevice.h',
> > +    'yaml_emitter.h',
> >      'yaml_parser.h',
> >  ])
> >
> > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> > new file mode 100644
> > index 000000000000..78196a7f147f
> > --- /dev/null
> > +++ b/include/libcamera/internal/yaml_emitter.h
> > @@ -0,0 +1,164 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +#include <string>
> > +#include <string_view>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/file.h>
> > +
> > +#include <yaml.h>
> > +
> > +namespace libcamera {
> > +
> > +class YamlDict;
> > +class YamlEvent;
> > +class YamlList;
> > +class YamlRoot;
> > +class YamlScalar;
> > +
> > +class YamlEmitter final
> > +{
> > +public:
> > +	~YamlEmitter();
> > +
> > +	static YamlRoot root(const std::string &path);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +	friend class YamlRoot;
> > +
> > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > +
> > +	YamlEmitter(const std::string &path);
> > +
> > +	void logError();
> > +	void init();
> > +	int emit();
> > +
> > +	File file_;
> > +	yaml_event_t event_;
> > +	yaml_emitter_t emitter_;
> > +};
> > +
> > +class YamlOutput
> > +{
> > +public:
> > +	bool valid() const { return !!emitter_; }
> > +
> > +protected:
> > +	YamlOutput() = default;
> > +
> > +	YamlOutput(YamlEmitter *emitter)
> > +		: emitter_(emitter)
> > +	{
> > +	}
> > +
> > +	YamlOutput &operator=(YamlOutput &&other)
> > +	{
> > +		emitter_ = other.emitter_;
> > +		other.emitter_ = nullptr;
> > +
> > +		return *this;
> > +	}
> > +
> > +	int emitScalar(std::string_view scalar);
> > +	int emitMappingStart();
> > +	int emitMappingEnd();
> > +	int emitSequenceStart();
> > +	int emitSequenceEnd();
> > +
> > +	YamlScalar scalar();
> > +	YamlDict dict();
> > +	YamlList list();
> > +
> > +	YamlEmitter *emitter_ = nullptr;
> > +	yaml_event_t event_;
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(YamlOutput)
> > +};
> > +
> > +class YamlRoot : public YamlOutput
> > +{
> > +public:
> > +	YamlRoot() = default;
> > +	~YamlRoot();
> > +
> > +	YamlRoot &operator=(YamlRoot &&other) = default;
> > +
> > +	YamlList list();
> > +	YamlDict dict();
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(YamlRoot);
> > +
> > +	friend class YamlEmitter;
> > +
> > +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> > +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> > +	{
> > +	}
> > +
> > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > +};
> > +
> > +class YamlScalar : public YamlOutput
> > +{
> > +public:
> > +	YamlScalar() = default;
> > +	~YamlScalar() = default;
> > +
> > +	void operator=(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlScalar(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlList : public YamlOutput
> > +{
> > +public:
> > +	YamlList() = default;
> > +	~YamlList();
> > +
> > +	YamlList &operator=(YamlList &&other) = default;
> > +
> > +	YamlList list();
> > +	YamlDict dict();
> > +	void scalar(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlList(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlDict : public YamlOutput
> > +{
> > +public:
> > +	YamlDict() = default;
> > +	~YamlDict();
> > +
> > +	YamlDict &operator=(YamlDict &&other) = default;
> > +
> > +	YamlList list(std::string_view key);
> > +	YamlDict dict(std::string_view key);
> > +
> > +	YamlScalar operator[](std::string_view key);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlDict(YamlEmitter *emitter);
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index aa9ab0291854..5b8af4103085 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
> >      'v4l2_pixelformat.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> > +    'yaml_emitter.cpp',
> >      'yaml_parser.cpp',
> >  ])
> >
> > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> > new file mode 100644
> > index 000000000000..fa3de91ce988
> > --- /dev/null
> > +++ b/src/libcamera/yaml_emitter.cpp
> > @@ -0,0 +1,577 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#include "libcamera/internal/yaml_emitter.h"
> > +
> > +#include <libcamera/base/log.h>
>
> Missing
>
> #include <libcamera/base/span.h>
>
> You should also include errno.h.
>
> > +
> > +/**
> > + * \file yaml_emitter.h
> > + * \brief A YAML emitter helper
> > + *
> > + * The YAML emitter helpers allow users to emit output in YAML format.
>
> I would outline here the key difference with a YAML writer.
>
>  * Unlike YAML writers that operate on a fully populated representation of the
>  * data, the YAML emitter outputs YAML data on the fly. It is suitable for
>  * outputting large amount of data with low overhead and no runtime heap
>  * allocation.
>

I like this

> > + *
> > + * To emit YAML users of the these helper classes create a root node with
> > + *
> > + * \code
> > +	std::string filePath("...");
> > +	auto root = YamlEmitter::root(filePath);
> > +   \endcode
> > + *
> > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > + * and YamlRoot::list() functions.
>
> I would write "and start emitting dictionaries and lists", "populating"
> sounds more like a YAML writer.
>
ditto

> > + *
> > + * The classes part of this file implement RAII-style handling of YAML
> > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > + * sequence start and mapping start events are emitted and once the instances
> > + * gets destroyed the corresponding sequence end and mapping end events are
> > + * emitted.
> > + *
> > + * From an initialized YamlRoot instance is possible to create YAML list and
> > + * dictionaries.
> > + *
> > + * \code
> > +	YamlDict dict = root.dict();
> > +	YamlList list = root.list();
> > +   \endcode
> > + *
> > + * YamlDict instances can be populated with scalars associated with a key
> > + *
> > + * \code
> > +	dict["key"] = "value";
> > +   \endcode
> > + *
> > + * and it is possible to create lists and dictionaries, associated with a key
> > + *
> > + * \code
> > +	YamlDict subDict = dict.dict("newDict");
> > +	YamlList subList = dict.list("newList");
> > +   \endcode
> > + *
> > + * YamlList instances can be populated with scalar elements
> > + *
> > + * \code
> > +	list.scalar("x");
> > +	list.scalar("y");
> > +   \endcode
> > + *
> > + * and with dictionaries and lists too
> > + *
> > + * \code
> > +	YamlDict subDict = list.dict();
> > +	YamlList subList = list.list();
> > +   \endcode
>
> The biggest issue with the API is that it can be easily misused:
>
> 	YamlDict list1 = dict.list("foo");
> 	YamlDict list2 = dict.list("bar");
> 	list1.scalar("0");
>
> I'm still annoyed that I haven't found a neat way to prevent this. I
> wonder if we could make it a bit safer by at least catching incorrect
> usage at runtime. What I'm thinking is

Yes, unfortunately I don't think we can easily catch this at build
time

>
> - When creating a child, recording the child pointer in the parent, and
>   the parent pointer in the child
> - If the parent already has a child when a new child is created, reset
>   the parent pointer of the previous child. This makes the previous
>   child invalid.
> - Any invalidation of a child needs to be propagated to its children.
> - Any operation on an invalid child would be caught and logged.
> - When a child is destroyed, if its parent is not null, reset the child
>   pointer in the parent to null.
> - If a parent is destroyed while it has a child pointer, reset the
>   child's parent pointer to null and log an error.
>

That should be enough, I wonder if we can make access to an invalid
childer a compiler error...

> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(YamlEmitter)
> > +
> > +namespace {
> > +
> > +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> > +{
> > +	File *file = static_cast<File *>(data);
> > +
> > +	Span<unsigned char> buf{ buffer, size };
> > +	ssize_t ret = file->write(buf);
> > +	if (ret < 0) {
> > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
>
> s/error :/error:/
>
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class YamlEmitter
> > + *
> > + * YAML helper classes entry point. This class allows to create a YamlRoot
> > + * instances, using the YamlEmitter::root() function, that users can populate
>
> s/instances/instance/
>
> > + * with lists, dictionaries and scalars.
> > + */
> > +
> > +YamlEmitter::YamlEmitter(const std::string &path)
> > +{
> > +	std::string filePath(path);
> > +	file_.setFileName(filePath);
>
> I don't think you need the local variable, you can write
>
> 	file_.setFileName(path);
>
> > +	file_.open(File::OpenModeFlag::WriteOnly);
> > +}
> > +
> > +/**
> > + * \brief Destroy the YamlEmitter
> > + */
>
> I think you can drop this, the documentation doesn't bring much, and the
> function isn't part of the API exposed to users anyway.
>

Isn't it a public function ??

> > +YamlEmitter::~YamlEmitter()
> > +{
> > +	yaml_event_delete(&event_);
> > +	yaml_emitter_delete(&emitter_);
> > +}
> > +
> > +/**
> > + * \brief Create an initialized instance of YamlRoot
> > + * \param[in] path The YAML output file path
> > + *
> > + * Create an initialized instance of the YamlRoot class that users can start
> > + * using and populating with scalers, lists and dictionaries.
>
> s/scalers/scalars/
>
> > + *
> > + * \return An initialized YamlRoot instance
> > + */
> > +YamlRoot YamlEmitter::root(const std::string &path)
>
> I asked in the review of the preview version if we should pass a
> std::ostream to make this more generic. I'm fine ignoring it for now, as
> it's unclear if we would have use cases for that.
>

I might have missed this. I recall I had a string_view most probably
here. Let's post-pone it for when we have such a use case.

> > +{
> > +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(path) };
> > +
> > +	emitter->init();
> > +
> > +	return YamlRoot(std::move(emitter));
> > +}
> > +
> > +void YamlEmitter::logError()
> > +{
> > +	switch (emitter_.error) {
> > +	case YAML_MEMORY_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Memory error: Not enough memory for emitting";
> > +		break;
> > +
> > +	case YAML_WRITER_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Writer error: " << emitter_.problem;
> > +		break;
> > +
> > +	case YAML_EMITTER_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Emitter error: " << emitter_.problem;
> > +		break;
> > +
> > +	default:
> > +		LOG(YamlEmitter, Error) << "Internal problem";
>
> "Internal error" ?
>

ack

> > +		break;
> > +	}
> > +}
> > +
> > +void YamlEmitter::init()
> > +{
> > +	yaml_emitter_initialize(&emitter_);
> > +	yaml_emitter_set_output(&emitter_, yamlWrite, &file_);
> > +
> > +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> > +	emit();
> > +
> > +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);
>
> nullptr ? Same elsewhere.
>
> > +	emit();
> > +}
> > +
> > +int YamlEmitter::emit()
> > +{
> > +	int ret = yaml_emitter_emit(&emitter_, &event_);
> > +	if (!ret) {
> > +		logError();
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \class YamlOutput
> > + *
> > + * The YamlOutput base class. From this class are derived the YamlScalar,
> > + * YamlList and YamlDict classes which are meant to be used by users of the
> > + * YAML emitter helpers.
> > + *
> > + * The YamlOutput base class provides functions to create YAML lists and
> > + * dictionaries and to populate them.
> > + *
> > + * Instances of this class cannot be instantiated directly by applications.
>
> s/Instances of this/This/
> s/ by applications// as none of this is exposed to applications :-)
>

Yeah, I mean users of this class

> > + */
> > +
> > +/**
> > + * \fn YamlOutput::valid()
> > + * \brief Check if a YamlOutput instance has been correctly initialized
> > + * \return True if the instance has been initialized, false otherwise
> > + */
>
> We usually document constructors first.
>
> > +
> > +/**
> > + * \fn YamlOutput::YamlOutput(YamlEmitter *emitter)
> > + * \brief Create a YamlOutput instance with an associated emitter
> > + * \param[in] emitter The YAML emitter
> > + */
> > +
> > +/**
> > + * \fn YamlOutput &YamlOutput::operator=(YamlOutput &&other)
> > + * \brief The move-assignment operator
> > + * \param[in] other The instance to be moved
> > + */
> > +
> > +/**
> > + * \brief Emit \a scalar as a YAML scalar
> > + * \param[in] scalar The element to emit
> > + * \return 0 in case of success, a negative error value otherwise
> > + */
> > +int YamlOutput::emitScalar(std::string_view scalar)
> > +{
> > +	if (!valid())
> > +		return -EINVAL;
> > +
> > +	const yaml_char_t *value = reinterpret_cast<const yaml_char_t *>
> > +				   (scalar.data());
> > +	yaml_scalar_event_initialize(&emitter_->event_, NULL, NULL, value,
> > +				     scalar.length(), true, false,
> > +				     YAML_PLAIN_SCALAR_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \brief Emit the mapping start YAML event
> > + * \return 0 in case of success, a negative error value otherwise
> > + */
> > +int YamlOutput::emitMappingStart()
> > +{
> > +	if (!valid())
> > +		return -EINVAL;
> > +
> > +	yaml_mapping_start_event_initialize(&emitter_->event_, NULL, NULL,
> > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \brief Emit the mapping end YAML event
> > + * \return 0 in case of success, a negative error value otherwise
> > + */
> > +int YamlOutput::emitMappingEnd()
> > +{
> > +	if (!valid())
> > +		return -EINVAL;
> > +
> > +	yaml_mapping_end_event_initialize(&emitter_->event_);
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \brief Emit the sequence start YAML event
> > + * \return 0 in case of success, a negative error value otherwise
> > + */
> > +int YamlOutput::emitSequenceStart()
> > +{
> > +	if (!valid())
> > +		return -EINVAL;
> > +
> > +	yaml_sequence_start_event_initialize(&emitter_->event_, NULL, NULL,
> > +					     true, YAML_BLOCK_SEQUENCE_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \brief Emit the sequence end YAML event
> > + * \return 0 in case of success, a negative error value otherwise
> > + */
> > +int YamlOutput::emitSequenceEnd()
> > +{
> > +	if (!valid())
> > +		return -EINVAL;
> > +
> > +	yaml_sequence_end_event_initialize(&emitter_->event_);
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \brief Create a scalar instance
> > + * \return An instance of YamlScalar
> > + */
> > +YamlScalar YamlOutput::scalar()
> > +{
> > +	return YamlScalar(emitter_);
> > +}
> > +
> > +/**
> > + * \brief Create a dictionary instance
> > + * \return An instance of YamlDict
> > + */
> > +YamlDict YamlOutput::dict()
> > +{
> > +	return YamlDict(emitter_);
> > +}
> > +
> > +/**
> > + * \brief Create a list instance
> > + * \return An instance of YamlList
> > + */
> > +YamlList YamlOutput::list()
> > +{
> > +	return YamlList(emitter_);
> > +}
> > +
> > +/**
> > + * \var YamlOutput::emitter_
> > + * \brief The emitter used by this YamlObject to output YAML events
> > + */
> > +
> > +/**
> > + * \var YamlOutput::event_
> > + * \brief The YAML event used by this YamlObject
> > + */
> > +
> > +/**
> > + * \class YamlRoot
> > + *
> > + * The YAML root node. A valid YamlRoot instance can only be created using the
> > + * YamlEmitter::root() function. The typical initialization pattern of users of
> > + * this class is similar to the one in the following example:
> > + *
> > + * \code
> > +	class YamlUser
> > +	{
> > +	public:
> > +		YamlUser();
> > +
> > +	private:
> > +		YamlRool root_;
> > +	};
> > +
> > +	YamlUser::YamlUser()
> > +	{
> > +		root_ = YamlEmitter::root("/path/to/yaml/file.yml");
> > +	}
> > +   \endcode
> > + *
> > + * A YamlRoot element can be populated with list and dictionaries.
> > + */
> > +
> > +/**
> > + * \fn YamlRoot::YamlRoot()
> > + * \brief Construct a YamlRoot instance without initializing it
> > + *
> > + * A YamlRoot instance can be created in non-initialized state typically to be
> > + * stored as a class member by the users of this class. In order to start using
> > + * and populating the YamlRoot instance a valid and initialized instance,
> > + * created using the YamlEmitter::root() function, has to be move-assigned to
> > + * the non-initialized instance.
> > + *
> > + * \code
> > +	YamlRoot root;
> > +
> > +	root = YamlEmitter::root("/path/to/yaml/file.yml");
> > +   \endcode
> > + */
> > +
> > +/**
> > + * \brief Delete a YamlRoot
> > + */
> > +YamlRoot::~YamlRoot()
> > +{
> > +	if (!valid())
> > +		return;
> > +
> > +	yaml_document_end_event_initialize(&emitter_->event_, 0);
> > +	emitterRoot_->emit();
> > +
> > +	yaml_stream_end_event_initialize(&emitter_->event_);
> > +	emitterRoot_->emit();
> > +}
> > +
> > +/**
> > + * \fn YamlRoot &YamlRoot::operator=(YamlRoot &&other)
> > + * \brief Move assignment operator
> > + *
> > + * Move-assign a YamlRoot instance. This function is typically used to assign an
> > + * initialized instance returned by YamlEmitter::root() to a non-initialized
> > + * one.
> > + *
> > + * \return A reference to this instance of YamlRoot
> > + */
> > +
> > +/**
> > + * \copydoc YamlOutput::dict()
> > + */
> > +YamlDict YamlRoot::dict()
> > +{
> > +	int ret = emitMappingStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +/**
> > + * \copydoc YamlOutput::list()
> > + */
> > +YamlList YamlRoot::list()
> > +{
> > +	int ret = emitSequenceStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +/**
> > + * \class YamlScalar
> > + *
> > + * A YamlScalar can be assigned to an std::string_view to emit them as YAML
> > + * elements.
> > + */
> > +
> > +/**
> > + * \brief Create a YamlScalar instance
> > + */
> > +YamlScalar::YamlScalar(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Emit \a scalar as a YAML scalar
> > + * \param[in] scalar The element to emit in the YAML output
> > + */
> > +void YamlScalar::operator=(std::string_view scalar)
> > +{
> > +	emitScalar(scalar);
> > +}
> > +
> > +/**
> > + * \class YamlList
> > + *
> > + * A YamlList can be populated with scalars and allows to create nested lists
> > + * and dictionaries.
> > + */
> > +
> > +/**
> > + * \brief Create a YamlList
> > + */
> > +YamlList::YamlList(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Destroy a YamlList instance
> > + */
> > +YamlList::~YamlList()
> > +{
> > +	emitSequenceEnd();
> > +}
> > +
> > +/**
> > + * \fn YamlList &YamlList::operator=(YamlList &&other)
> > + * \brief Move-assignment operator
> > + * \param[inout] other The instance to move
> > + */
> > +
> > +/**
> > + * \brief Append \a scalar to the list
> > + * \param[in] scalar The element to append to the list
> > + */
> > +void YamlList::scalar(std::string_view scalar)
> > +{
> > +	emitScalar(scalar);
> > +}
> > +
> > +/**
> > + * \copydoc YamlOutput::list()
> > + */
> > +YamlList YamlList::list()
> > +{
> > +	int ret = emitSequenceStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +/**
> > + * \copydoc YamlOutput::dict()
> > + */
> > +YamlDict YamlList::dict()
> > +{
> > +	int ret = emitMappingStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +/**
> > + * \class YamlDict
> > + *
> > + * A YamlDict can be populated with scalars using operator[] and allows to
> > + * create other lists and dictionaries associated with a key.
> > + */
> > +
> > +/**
> > + * \fn YamlDict::YamlDict()
> > + * \brief Create a non-initialized instance of a YamlDict
> > + */
> > +
> > +YamlDict::YamlDict(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Destroy a YamlDict instance
> > + */
> > +YamlDict::~YamlDict()
> > +{
> > +	emitMappingEnd();
> > +}
> > +
> > +/**
> > + * \fn YamlDict &YamlDict::operator=(YamlDict &&other)
> > + * \brief Move-assignment operator
> > + * \param[inout] other The instance to move
>
> I think we usually use param[in] for move constructors and assignment
> operators. Granted, it's a bit borderline.
>

ack

> > + */
> > +
> > +/**
> > + * \copydoc YamlOutput::list()
>
> That won't document the key parameter.
>
> > + */
> > +YamlList YamlDict::list(std::string_view key)
> > +{
> > +	int ret = emitScalar(key);
> > +	if (ret)
> > +		return {};
> > +
> > +	ret = emitSequenceStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +/**
> > + * \copydoc YamlOutput::dict()
>
> Same here.
>

I'll manually copy the documentation

> > + */
> > +YamlDict YamlDict::dict(std::string_view key)
> > +{
> > +	int ret = emitScalar(key);
> > +	if (ret)
> > +		return {};
> > +
> > +	ret = emitMappingStart();
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +/**
> > + * \brief Create a scalar associated with \a key in the dictionary
> > + * \param[in] key The key associated with the newly created scalar
> > + * \return A YamlScalar that application can use to output text
> > + */
> > +YamlScalar YamlDict::operator[](std::string_view key)
>
> I'm still not entirely thrilled by this, for a few reasons. None are
> showstoppers, you can decide what to do.
>
> First there's an asymmetry in the API. To emit scalars on lists you have
> a scalar() function, while here you use the [] operator. Similarly, in
> the YamlDict class, you have explicitly named functions to emit a list
> or dictionary, but not for scalars. Now asymmetries are not always bad,
> sometimes specific APIs are best for specific jobs.

To be hones I find the usage of operator[] to emit a scalar
(associated with a key) quite neat as it directly connect to the
'dictionary' API (or a map, as this is C++ and not Python).

You're right, it's asymmetric, but in lists you can just emit a
scalar, in dict you always associate it (as well as anything else)
with a key. Operator[] makes sure you pass a key in :)

>
> Another API design point that bothers me possibly a bit more is the
> difference in behaviour between YamlDict::operator[]() and
> std::map::operator[](). Using the [] operators, users could expect that
>
> 	dict["foo"] = "bar";
> 	dict["foo"] = "baz";
>
> will result in the second statement overwriting the first one. The
> YamlDict::operator[]() API is a bit misleading. As it's an internal
> class it's less of an issue than it would be for the libcamera public
> API, but still.

As we don't retain data but emit them as soon as we can, yes, this
won't work as std::map.

>
> Finally, returning a YamlScalar object opens the door for misuse. The
> following code will compile, but is invalid:
>
> 	YamlScalar scalar = dict["foo"];
> 	dict["bar"] = "0";
> 	scalar = "1";
>
> Using an explicit function would solve all this:
>
> 	dict.scalar("foo", "0");
>

I can indeed use a 'scalar' function if it's safer

> The YamlScalar class could then be moved from the .h file to the .cpp
> file, and possibly even dropped completely.
>

yes, users now do not need to manage scalars directly now

Thanks
  j

> > +{
> > +	int ret = emitScalar(key);
> > +	if (ret)
> > +		return {};
> > +
> > +	return YamlOutput::scalar();
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

On Fri, Nov 29, 2024 at 09:35:06AM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> > On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > > Implement a helpers to allow outputting text in YAML format.
> >
> > s/helpers/helper/
> >
> > > The class of helpers allows to create list and dictionaries and emit
> >
> > s/list/lists/
> >
> > > scalar values starting from a YamlRoot object initialized with
> > > a file path.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/meson.build    |   1 +
> > >  include/libcamera/internal/yaml_emitter.h | 164 ++++++
> > >  src/libcamera/meson.build                 |   1 +
> > >  src/libcamera/yaml_emitter.cpp            | 577 ++++++++++++++++++++++
> > >  4 files changed, 743 insertions(+)
> > >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> > >  create mode 100644 src/libcamera/yaml_emitter.cpp
> > >
> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > index 1c5eef9cab80..7533b075fde2 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
> > >      'v4l2_pixelformat.h',
> > >      'v4l2_subdevice.h',
> > >      'v4l2_videodevice.h',
> > > +    'yaml_emitter.h',
> > >      'yaml_parser.h',
> > >  ])
> > >
> > > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> > > new file mode 100644
> > > index 000000000000..78196a7f147f
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/yaml_emitter.h
> > > @@ -0,0 +1,164 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board Oy
> > > + *
> > > + * libcamera YAML emitter helper
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <memory>
> > > +#include <string>
> > > +#include <string_view>
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/base/file.h>
> > > +
> > > +#include <yaml.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class YamlDict;
> > > +class YamlEvent;
> > > +class YamlList;
> > > +class YamlRoot;
> > > +class YamlScalar;
> > > +
> > > +class YamlEmitter final
> > > +{
> > > +public:
> > > +	~YamlEmitter();
> > > +
> > > +	static YamlRoot root(const std::string &path);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +	friend class YamlRoot;
> > > +
> > > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > > +
> > > +	YamlEmitter(const std::string &path);
> > > +
> > > +	void logError();
> > > +	void init();
> > > +	int emit();
> > > +
> > > +	File file_;
> > > +	yaml_event_t event_;
> > > +	yaml_emitter_t emitter_;
> > > +};
> > > +
> > > +class YamlOutput
> > > +{
> > > +public:
> > > +	bool valid() const { return !!emitter_; }
> > > +
> > > +protected:
> > > +	YamlOutput() = default;
> > > +
> > > +	YamlOutput(YamlEmitter *emitter)
> > > +		: emitter_(emitter)
> > > +	{
> > > +	}
> > > +
> > > +	YamlOutput &operator=(YamlOutput &&other)
> > > +	{
> > > +		emitter_ = other.emitter_;
> > > +		other.emitter_ = nullptr;
> > > +
> > > +		return *this;
> > > +	}
> > > +
> > > +	int emitScalar(std::string_view scalar);
> > > +	int emitMappingStart();
> > > +	int emitMappingEnd();
> > > +	int emitSequenceStart();
> > > +	int emitSequenceEnd();
> > > +
> > > +	YamlScalar scalar();
> > > +	YamlDict dict();
> > > +	YamlList list();
> > > +
> > > +	YamlEmitter *emitter_ = nullptr;
> > > +	yaml_event_t event_;
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(YamlOutput)
> > > +};
> > > +
> > > +class YamlRoot : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlRoot() = default;
> > > +	~YamlRoot();
> > > +
> > > +	YamlRoot &operator=(YamlRoot &&other) = default;
> > > +
> > > +	YamlList list();
> > > +	YamlDict dict();
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(YamlRoot);
> > > +
> > > +	friend class YamlEmitter;
> > > +
> > > +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> > > +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> > > +	{
> > > +	}
> > > +
> > > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > > +};
> > > +
> > > +class YamlScalar : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlScalar() = default;
> > > +	~YamlScalar() = default;
> > > +
> > > +	void operator=(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlScalar(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlList : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlList() = default;
> > > +	~YamlList();
> > > +
> > > +	YamlList &operator=(YamlList &&other) = default;
> > > +
> > > +	YamlList list();
> > > +	YamlDict dict();
> > > +	void scalar(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlList(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlDict : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlDict() = default;
> > > +	~YamlDict();
> > > +
> > > +	YamlDict &operator=(YamlDict &&other) = default;
> > > +
> > > +	YamlList list(std::string_view key);
> > > +	YamlDict dict(std::string_view key);
> > > +
> > > +	YamlScalar operator[](std::string_view key);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlDict(YamlEmitter *emitter);
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index aa9ab0291854..5b8af4103085 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
> > >      'v4l2_pixelformat.cpp',
> > >      'v4l2_subdevice.cpp',
> > >      'v4l2_videodevice.cpp',
> > > +    'yaml_emitter.cpp',
> > >      'yaml_parser.cpp',
> > >  ])
> > >
> > > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> > > new file mode 100644
> > > index 000000000000..fa3de91ce988
> > > --- /dev/null
> > > +++ b/src/libcamera/yaml_emitter.cpp
> > > @@ -0,0 +1,577 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board Oy
> > > + *
> > > + * libcamera YAML emitter helper
> > > + */
> > > +
> > > +#include "libcamera/internal/yaml_emitter.h"
> > > +
> > > +#include <libcamera/base/log.h>
> >
> > Missing
> >
> > #include <libcamera/base/span.h>
> >
> > You should also include errno.h.
> >
> > > +
> > > +/**
> > > + * \file yaml_emitter.h
> > > + * \brief A YAML emitter helper
> > > + *
> > > + * The YAML emitter helpers allow users to emit output in YAML format.
> >
> > I would outline here the key difference with a YAML writer.
> >
> >  * Unlike YAML writers that operate on a fully populated representation of the
> >  * data, the YAML emitter outputs YAML data on the fly. It is suitable for
> >  * outputting large amount of data with low overhead and no runtime heap
> >  * allocation.
> 
> I like this
> 
> > > + *
> > > + * To emit YAML users of the these helper classes create a root node with
> > > + *
> > > + * \code
> > > +	std::string filePath("...");
> > > +	auto root = YamlEmitter::root(filePath);
> > > +   \endcode
> > > + *
> > > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > > + * and YamlRoot::list() functions.
> >
> > I would write "and start emitting dictionaries and lists", "populating"
> > sounds more like a YAML writer.
> >
> ditto
> 
> > > + *
> > > + * The classes part of this file implement RAII-style handling of YAML
> > > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > > + * sequence start and mapping start events are emitted and once the instances
> > > + * gets destroyed the corresponding sequence end and mapping end events are
> > > + * emitted.
> > > + *
> > > + * From an initialized YamlRoot instance is possible to create YAML list and
> > > + * dictionaries.
> > > + *
> > > + * \code
> > > +	YamlDict dict = root.dict();
> > > +	YamlList list = root.list();
> > > +   \endcode
> > > + *
> > > + * YamlDict instances can be populated with scalars associated with a key
> > > + *
> > > + * \code
> > > +	dict["key"] = "value";
> > > +   \endcode
> > > + *
> > > + * and it is possible to create lists and dictionaries, associated with a key
> > > + *
> > > + * \code
> > > +	YamlDict subDict = dict.dict("newDict");
> > > +	YamlList subList = dict.list("newList");
> > > +   \endcode
> > > + *
> > > + * YamlList instances can be populated with scalar elements
> > > + *
> > > + * \code
> > > +	list.scalar("x");
> > > +	list.scalar("y");
> > > +   \endcode
> > > + *
> > > + * and with dictionaries and lists too
> > > + *
> > > + * \code
> > > +	YamlDict subDict = list.dict();
> > > +	YamlList subList = list.list();
> > > +   \endcode
> >
> > The biggest issue with the API is that it can be easily misused:
> >
> > 	YamlDict list1 = dict.list("foo");
> > 	YamlDict list2 = dict.list("bar");
> > 	list1.scalar("0");
> >
> > I'm still annoyed that I haven't found a neat way to prevent this. I
> > wonder if we could make it a bit safer by at least catching incorrect
> > usage at runtime. What I'm thinking is
> 
> Yes, unfortunately I don't think we can easily catch this at build
> time
> 
> > - When creating a child, recording the child pointer in the parent, and
> >   the parent pointer in the child
> > - If the parent already has a child when a new child is created, reset
> >   the parent pointer of the previous child. This makes the previous
> >   child invalid.
> > - Any invalidation of a child needs to be propagated to its children.
> > - Any operation on an invalid child would be caught and logged.
> > - When a child is destroyed, if its parent is not null, reset the child
> >   pointer in the parent to null.
> > - If a parent is destroyed while it has a child pointer, reset the
> >   child's parent pointer to null and log an error.
> >
> 
> That should be enough, I wonder if we can make access to an invalid
> childer a compiler error...

I'd love to, but haven't found a nice way to do so :-S

> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(YamlEmitter)
> > > +
> > > +namespace {
> > > +
> > > +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> > > +{
> > > +	File *file = static_cast<File *>(data);
> > > +
> > > +	Span<unsigned char> buf{ buffer, size };
> > > +	ssize_t ret = file->write(buf);
> > > +	if (ret < 0) {
> > > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> >
> > s/error :/error:/
> >
> > > +		return 0;
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \class YamlEmitter
> > > + *
> > > + * YAML helper classes entry point. This class allows to create a YamlRoot
> > > + * instances, using the YamlEmitter::root() function, that users can populate
> >
> > s/instances/instance/
> >
> > > + * with lists, dictionaries and scalars.
> > > + */
> > > +
> > > +YamlEmitter::YamlEmitter(const std::string &path)
> > > +{
> > > +	std::string filePath(path);
> > > +	file_.setFileName(filePath);
> >
> > I don't think you need the local variable, you can write
> >
> > 	file_.setFileName(path);
> >
> > > +	file_.open(File::OpenModeFlag::WriteOnly);
> > > +}
> > > +
> > > +/**
> > > + * \brief Destroy the YamlEmitter
> > > + */
> >
> > I think you can drop this, the documentation doesn't bring much, and the
> > function isn't part of the API exposed to users anyway.
> 
> Isn't it a public function ??

It is public, but isn't called by users. The YamlEmitter instance is
created internally by YamlEmitter::root(), and stored in the YamlRoot
class. The user never interacts with the YamlEmitter instance directly.

> > > +YamlEmitter::~YamlEmitter()
> > > +{
> > > +	yaml_event_delete(&event_);
> > > +	yaml_emitter_delete(&emitter_);
> > > +}
> > > +
> > > +/**
> > > + * \brief Create an initialized instance of YamlRoot
> > > + * \param[in] path The YAML output file path
> > > + *
> > > + * Create an initialized instance of the YamlRoot class that users can start
> > > + * using and populating with scalers, lists and dictionaries.
> >
> > s/scalers/scalars/
> >
> > > + *
> > > + * \return An initialized YamlRoot instance
> > > + */
> > > +YamlRoot YamlEmitter::root(const std::string &path)
> >
> > I asked in the review of the preview version if we should pass a
> > std::ostream to make this more generic. I'm fine ignoring it for now, as
> > it's unclear if we would have use cases for that.
> 
> I might have missed this. I recall I had a string_view most probably
> here. Let's post-pone it for when we have such a use case.
> 
> > > +{
> > > +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(path) };
> > > +
> > > +	emitter->init();
> > > +
> > > +	return YamlRoot(std::move(emitter));
> > > +}
> > > +
> > > +void YamlEmitter::logError()
> > > +{
> > > +	switch (emitter_.error) {
> > > +	case YAML_MEMORY_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Memory error: Not enough memory for emitting";
> > > +		break;
> > > +
> > > +	case YAML_WRITER_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Writer error: " << emitter_.problem;
> > > +		break;
> > > +
> > > +	case YAML_EMITTER_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Emitter error: " << emitter_.problem;
> > > +		break;
> > > +
> > > +	default:
> > > +		LOG(YamlEmitter, Error) << "Internal problem";
> >
> > "Internal error" ?
> 
> ack
> 
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +void YamlEmitter::init()
> > > +{
> > > +	yaml_emitter_initialize(&emitter_);
> > > +	yaml_emitter_set_output(&emitter_, yamlWrite, &file_);
> > > +
> > > +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> > > +	emit();
> > > +
> > > +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);
> >
> > nullptr ? Same elsewhere.
> >
> > > +	emit();
> > > +}
> > > +
> > > +int YamlEmitter::emit()
> > > +{
> > > +	int ret = yaml_emitter_emit(&emitter_, &event_);
> > > +	if (!ret) {
> > > +		logError();
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \class YamlOutput
> > > + *
> > > + * The YamlOutput base class. From this class are derived the YamlScalar,
> > > + * YamlList and YamlDict classes which are meant to be used by users of the
> > > + * YAML emitter helpers.
> > > + *
> > > + * The YamlOutput base class provides functions to create YAML lists and
> > > + * dictionaries and to populate them.
> > > + *
> > > + * Instances of this class cannot be instantiated directly by applications.
> >
> > s/Instances of this/This/
> > s/ by applications// as none of this is exposed to applications :-)
> 
> Yeah, I mean users of this class
> 
> > > + */
> > > +
> > > +/**
> > > + * \fn YamlOutput::valid()
> > > + * \brief Check if a YamlOutput instance has been correctly initialized
> > > + * \return True if the instance has been initialized, false otherwise
> > > + */
> >
> > We usually document constructors first.
> >
> > > +
> > > +/**
> > > + * \fn YamlOutput::YamlOutput(YamlEmitter *emitter)
> > > + * \brief Create a YamlOutput instance with an associated emitter
> > > + * \param[in] emitter The YAML emitter
> > > + */
> > > +
> > > +/**
> > > + * \fn YamlOutput &YamlOutput::operator=(YamlOutput &&other)
> > > + * \brief The move-assignment operator
> > > + * \param[in] other The instance to be moved
> > > + */
> > > +
> > > +/**
> > > + * \brief Emit \a scalar as a YAML scalar
> > > + * \param[in] scalar The element to emit
> > > + * \return 0 in case of success, a negative error value otherwise
> > > + */
> > > +int YamlOutput::emitScalar(std::string_view scalar)
> > > +{
> > > +	if (!valid())
> > > +		return -EINVAL;
> > > +
> > > +	const yaml_char_t *value = reinterpret_cast<const yaml_char_t *>
> > > +				   (scalar.data());
> > > +	yaml_scalar_event_initialize(&emitter_->event_, NULL, NULL, value,
> > > +				     scalar.length(), true, false,
> > > +				     YAML_PLAIN_SCALAR_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit the mapping start YAML event
> > > + * \return 0 in case of success, a negative error value otherwise
> > > + */
> > > +int YamlOutput::emitMappingStart()
> > > +{
> > > +	if (!valid())
> > > +		return -EINVAL;
> > > +
> > > +	yaml_mapping_start_event_initialize(&emitter_->event_, NULL, NULL,
> > > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit the mapping end YAML event
> > > + * \return 0 in case of success, a negative error value otherwise
> > > + */
> > > +int YamlOutput::emitMappingEnd()
> > > +{
> > > +	if (!valid())
> > > +		return -EINVAL;
> > > +
> > > +	yaml_mapping_end_event_initialize(&emitter_->event_);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit the sequence start YAML event
> > > + * \return 0 in case of success, a negative error value otherwise
> > > + */
> > > +int YamlOutput::emitSequenceStart()
> > > +{
> > > +	if (!valid())
> > > +		return -EINVAL;
> > > +
> > > +	yaml_sequence_start_event_initialize(&emitter_->event_, NULL, NULL,
> > > +					     true, YAML_BLOCK_SEQUENCE_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit the sequence end YAML event
> > > + * \return 0 in case of success, a negative error value otherwise
> > > + */
> > > +int YamlOutput::emitSequenceEnd()
> > > +{
> > > +	if (!valid())
> > > +		return -EINVAL;
> > > +
> > > +	yaml_sequence_end_event_initialize(&emitter_->event_);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a scalar instance
> > > + * \return An instance of YamlScalar
> > > + */
> > > +YamlScalar YamlOutput::scalar()
> > > +{
> > > +	return YamlScalar(emitter_);
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a dictionary instance
> > > + * \return An instance of YamlDict
> > > + */
> > > +YamlDict YamlOutput::dict()
> > > +{
> > > +	return YamlDict(emitter_);
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a list instance
> > > + * \return An instance of YamlList
> > > + */
> > > +YamlList YamlOutput::list()
> > > +{
> > > +	return YamlList(emitter_);
> > > +}
> > > +
> > > +/**
> > > + * \var YamlOutput::emitter_
> > > + * \brief The emitter used by this YamlObject to output YAML events
> > > + */
> > > +
> > > +/**
> > > + * \var YamlOutput::event_
> > > + * \brief The YAML event used by this YamlObject
> > > + */
> > > +
> > > +/**
> > > + * \class YamlRoot
> > > + *
> > > + * The YAML root node. A valid YamlRoot instance can only be created using the
> > > + * YamlEmitter::root() function. The typical initialization pattern of users of
> > > + * this class is similar to the one in the following example:
> > > + *
> > > + * \code
> > > +	class YamlUser
> > > +	{
> > > +	public:
> > > +		YamlUser();
> > > +
> > > +	private:
> > > +		YamlRool root_;
> > > +	};
> > > +
> > > +	YamlUser::YamlUser()
> > > +	{
> > > +		root_ = YamlEmitter::root("/path/to/yaml/file.yml");
> > > +	}
> > > +   \endcode
> > > + *
> > > + * A YamlRoot element can be populated with list and dictionaries.
> > > + */
> > > +
> > > +/**
> > > + * \fn YamlRoot::YamlRoot()
> > > + * \brief Construct a YamlRoot instance without initializing it
> > > + *
> > > + * A YamlRoot instance can be created in non-initialized state typically to be
> > > + * stored as a class member by the users of this class. In order to start using
> > > + * and populating the YamlRoot instance a valid and initialized instance,
> > > + * created using the YamlEmitter::root() function, has to be move-assigned to
> > > + * the non-initialized instance.
> > > + *
> > > + * \code
> > > +	YamlRoot root;
> > > +
> > > +	root = YamlEmitter::root("/path/to/yaml/file.yml");
> > > +   \endcode
> > > + */
> > > +
> > > +/**
> > > + * \brief Delete a YamlRoot
> > > + */
> > > +YamlRoot::~YamlRoot()
> > > +{
> > > +	if (!valid())
> > > +		return;
> > > +
> > > +	yaml_document_end_event_initialize(&emitter_->event_, 0);
> > > +	emitterRoot_->emit();
> > > +
> > > +	yaml_stream_end_event_initialize(&emitter_->event_);
> > > +	emitterRoot_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \fn YamlRoot &YamlRoot::operator=(YamlRoot &&other)
> > > + * \brief Move assignment operator
> > > + *
> > > + * Move-assign a YamlRoot instance. This function is typically used to assign an
> > > + * initialized instance returned by YamlEmitter::root() to a non-initialized
> > > + * one.
> > > + *
> > > + * \return A reference to this instance of YamlRoot
> > > + */
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::dict()
> > > + */
> > > +YamlDict YamlRoot::dict()
> > > +{
> > > +	int ret = emitMappingStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::list()
> > > + */
> > > +YamlList YamlRoot::list()
> > > +{
> > > +	int ret = emitSequenceStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +/**
> > > + * \class YamlScalar
> > > + *
> > > + * A YamlScalar can be assigned to an std::string_view to emit them as YAML
> > > + * elements.
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a YamlScalar instance
> > > + */
> > > +YamlScalar::YamlScalar(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit \a scalar as a YAML scalar
> > > + * \param[in] scalar The element to emit in the YAML output
> > > + */
> > > +void YamlScalar::operator=(std::string_view scalar)
> > > +{
> > > +	emitScalar(scalar);
> > > +}
> > > +
> > > +/**
> > > + * \class YamlList
> > > + *
> > > + * A YamlList can be populated with scalars and allows to create nested lists
> > > + * and dictionaries.
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a YamlList
> > > + */
> > > +YamlList::YamlList(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Destroy a YamlList instance
> > > + */
> > > +YamlList::~YamlList()
> > > +{
> > > +	emitSequenceEnd();
> > > +}
> > > +
> > > +/**
> > > + * \fn YamlList &YamlList::operator=(YamlList &&other)
> > > + * \brief Move-assignment operator
> > > + * \param[inout] other The instance to move
> > > + */
> > > +
> > > +/**
> > > + * \brief Append \a scalar to the list
> > > + * \param[in] scalar The element to append to the list
> > > + */
> > > +void YamlList::scalar(std::string_view scalar)
> > > +{
> > > +	emitScalar(scalar);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::list()
> > > + */
> > > +YamlList YamlList::list()
> > > +{
> > > +	int ret = emitSequenceStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::dict()
> > > + */
> > > +YamlDict YamlList::dict()
> > > +{
> > > +	int ret = emitMappingStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +/**
> > > + * \class YamlDict
> > > + *
> > > + * A YamlDict can be populated with scalars using operator[] and allows to
> > > + * create other lists and dictionaries associated with a key.
> > > + */
> > > +
> > > +/**
> > > + * \fn YamlDict::YamlDict()
> > > + * \brief Create a non-initialized instance of a YamlDict
> > > + */
> > > +
> > > +YamlDict::YamlDict(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Destroy a YamlDict instance
> > > + */
> > > +YamlDict::~YamlDict()
> > > +{
> > > +	emitMappingEnd();
> > > +}
> > > +
> > > +/**
> > > + * \fn YamlDict &YamlDict::operator=(YamlDict &&other)
> > > + * \brief Move-assignment operator
> > > + * \param[inout] other The instance to move
> >
> > I think we usually use param[in] for move constructors and assignment
> > operators. Granted, it's a bit borderline.
> 
> ack
> 
> > > + */
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::list()
> >
> > That won't document the key parameter.
> >
> > > + */
> > > +YamlList YamlDict::list(std::string_view key)
> > > +{
> > > +	int ret = emitScalar(key);
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	ret = emitSequenceStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +/**
> > > + * \copydoc YamlOutput::dict()
> >
> > Same here.
> 
> I'll manually copy the documentation
> 
> > > + */
> > > +YamlDict YamlDict::dict(std::string_view key)
> > > +{
> > > +	int ret = emitScalar(key);
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	ret = emitMappingStart();
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a scalar associated with \a key in the dictionary
> > > + * \param[in] key The key associated with the newly created scalar
> > > + * \return A YamlScalar that application can use to output text
> > > + */
> > > +YamlScalar YamlDict::operator[](std::string_view key)
> >
> > I'm still not entirely thrilled by this, for a few reasons. None are
> > showstoppers, you can decide what to do.
> >
> > First there's an asymmetry in the API. To emit scalars on lists you have
> > a scalar() function, while here you use the [] operator. Similarly, in
> > the YamlDict class, you have explicitly named functions to emit a list
> > or dictionary, but not for scalars. Now asymmetries are not always bad,
> > sometimes specific APIs are best for specific jobs.
> 
> To be hones I find the usage of operator[] to emit a scalar
> (associated with a key) quite neat as it directly connect to the
> 'dictionary' API (or a map, as this is C++ and not Python).
> 
> You're right, it's asymmetric, but in lists you can just emit a
> scalar, in dict you always associate it (as well as anything else)
> with a key. Operator[] makes sure you pass a key in :)
> 
> >
> > Another API design point that bothers me possibly a bit more is the
> > difference in behaviour between YamlDict::operator[]() and
> > std::map::operator[](). Using the [] operators, users could expect that
> >
> > 	dict["foo"] = "bar";
> > 	dict["foo"] = "baz";
> >
> > will result in the second statement overwriting the first one. The
> > YamlDict::operator[]() API is a bit misleading. As it's an internal
> > class it's less of an issue than it would be for the libcamera public
> > API, but still.
> 
> As we don't retain data but emit them as soon as we can, yes, this
> won't work as std::map.
> 
> >
> > Finally, returning a YamlScalar object opens the door for misuse. The
> > following code will compile, but is invalid:
> >
> > 	YamlScalar scalar = dict["foo"];
> > 	dict["bar"] = "0";
> > 	scalar = "1";
> >
> > Using an explicit function would solve all this:
> >
> > 	dict.scalar("foo", "0");
> >
> 
> I can indeed use a 'scalar' function if it's safer

I think it would be :-)

> > The YamlScalar class could then be moved from the .h file to the .cpp
> > file, and possibly even dropped completely.
> 
> yes, users now do not need to manage scalars directly now
> 
> > > +{
> > > +	int ret = emitScalar(key);
> > > +	if (ret)
> > > +		return {};
> > > +
> > > +	return YamlOutput::scalar();
> > > +}
> > > +
> > > +} /* namespace libcamera */

Hi


2024. november 29., péntek 09:44 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> On Fri, Nov 29, 2024 at 09:35:06AM +0100, Jacopo Mondi wrote:
> > On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> > > On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > > > Implement a helpers to allow outputting text in YAML format.
> > > [...]
> > > > + *
> > > > + * To emit YAML users of the these helper classes create a root node with
> > > > + *
> > > > + * \code
> > > > +	std::string filePath("...");
> > > > +	auto root = YamlEmitter::root(filePath);
> > > > +   \endcode
> > > > + *
> > > > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > > > + * and YamlRoot::list() functions.
> > >
> > > I would write "and start emitting dictionaries and lists", "populating"
> > > sounds more like a YAML writer.
> > >
> > ditto
> >
> > > > + *
> > > > + * The classes part of this file implement RAII-style handling of YAML
> > > > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > > > + * sequence start and mapping start events are emitted and once the instances
> > > > + * gets destroyed the corresponding sequence end and mapping end events are
> > > > + * emitted.
> > > > + *
> > > > + * From an initialized YamlRoot instance is possible to create YAML list and
> > > > + * dictionaries.
> > > > + *
> > > > + * \code
> > > > +	YamlDict dict = root.dict();
> > > > +	YamlList list = root.list();
> > > > +   \endcode
> > > > + *
> > > > + * YamlDict instances can be populated with scalars associated with a key
> > > > + *
> > > > + * \code
> > > > +	dict["key"] = "value";
> > > > +   \endcode
> > > > + *
> > > > + * and it is possible to create lists and dictionaries, associated with a key
> > > > + *
> > > > + * \code
> > > > +	YamlDict subDict = dict.dict("newDict");
> > > > +	YamlList subList = dict.list("newList");
> > > > +   \endcode
> > > > + *
> > > > + * YamlList instances can be populated with scalar elements
> > > > + *
> > > > + * \code
> > > > +	list.scalar("x");
> > > > +	list.scalar("y");
> > > > +   \endcode
> > > > + *
> > > > + * and with dictionaries and lists too
> > > > + *
> > > > + * \code
> > > > +	YamlDict subDict = list.dict();
> > > > +	YamlList subList = list.list();
> > > > +   \endcode
> > >
> > > The biggest issue with the API is that it can be easily misused:
> > >
> > > 	YamlDict list1 = dict.list("foo");
> > > 	YamlDict list2 = dict.list("bar");
> > > 	list1.scalar("0");
> > >
> > > I'm still annoyed that I haven't found a neat way to prevent this. I
> > > wonder if we could make it a bit safer by at least catching incorrect
> > > usage at runtime. What I'm thinking is
> >
> > Yes, unfortunately I don't think we can easily catch this at build
> > time
> >
> > > - When creating a child, recording the child pointer in the parent, and
> > >   the parent pointer in the child
> > > - If the parent already has a child when a new child is created, reset
> > >   the parent pointer of the previous child. This makes the previous
> > >   child invalid.
> > > - Any invalidation of a child needs to be propagated to its children.
> > > - Any operation on an invalid child would be caught and logged.
> > > - When a child is destroyed, if its parent is not null, reset the child
> > >   pointer in the parent to null.
> > > - If a parent is destroyed while it has a child pointer, reset the
> > >   child's parent pointer to null and log an error.
> > >
> >
> > That should be enough, I wonder if we can make access to an invalid
> > childer a compiler error...
> 
> I'd love to, but haven't found a nice way to do so :-S
> [...]

My guess is that the C++ type system is "too weak" in this sense, to implement
the kind of checking you envision.

If one is willing to trade indentation for a smaller chance of misuse, then one could:
(Or at least I think such an API where the structure of the resulting YAML file
 is in a sense encapsulated in the layout of the source code leads to fewer misuses.)

   YamlDict root;

   root.dict("foo", [](auto& d) {
     d.list("x", [](auto& l) {
       l.scalar(1);
       l.scalar(true);
     });
     d.scalar("y", 42);
   });

   root.list("bar", [](auto&) {
     ...
   });

   /* the methods could return an appropriate reference to make some kind of chaining possible: */
   
   YamlDict()
     .dict("foo", [](auto& d) {
        d
          .list("x", [](auto& l) {
             l.scalar(1)
              .scalar(true);
          })
          .scalar("y", 42);
      })
     .list("bar", [](auto&) { ... });

Or one could even go fully in on some kind of chaining:

YamlDict()
	.dict("foo")
		.list("x")
			.scalar(1)
			.scalar(true)
		.finish() /* end list */
		.scalar()
	.finish() /* end dict */
	.list("bar")
		...


I am not sure if this has already been considered, sorry if it has been.

Regards,
Barnabás Pőcze

Hi Barnabás

On Fri, Nov 29, 2024 at 05:28:01PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. november 29., péntek 09:44 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:
>
> > On Fri, Nov 29, 2024 at 09:35:06AM +0100, Jacopo Mondi wrote:
> > > On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> > > > On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > > > > Implement a helpers to allow outputting text in YAML format.
> > > > [...]
> > > > > + *
> > > > > + * To emit YAML users of the these helper classes create a root node with
> > > > > + *
> > > > > + * \code
> > > > > +	std::string filePath("...");
> > > > > +	auto root = YamlEmitter::root(filePath);
> > > > > +   \endcode
> > > > > + *
> > > > > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > > > > + * and YamlRoot::list() functions.
> > > >
> > > > I would write "and start emitting dictionaries and lists", "populating"
> > > > sounds more like a YAML writer.
> > > >
> > > ditto
> > >
> > > > > + *
> > > > > + * The classes part of this file implement RAII-style handling of YAML
> > > > > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > > > > + * sequence start and mapping start events are emitted and once the instances
> > > > > + * gets destroyed the corresponding sequence end and mapping end events are
> > > > > + * emitted.
> > > > > + *
> > > > > + * From an initialized YamlRoot instance is possible to create YAML list and
> > > > > + * dictionaries.
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict dict = root.dict();
> > > > > +	YamlList list = root.list();
> > > > > +   \endcode
> > > > > + *
> > > > > + * YamlDict instances can be populated with scalars associated with a key
> > > > > + *
> > > > > + * \code
> > > > > +	dict["key"] = "value";
> > > > > +   \endcode
> > > > > + *
> > > > > + * and it is possible to create lists and dictionaries, associated with a key
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict subDict = dict.dict("newDict");
> > > > > +	YamlList subList = dict.list("newList");
> > > > > +   \endcode
> > > > > + *
> > > > > + * YamlList instances can be populated with scalar elements
> > > > > + *
> > > > > + * \code
> > > > > +	list.scalar("x");
> > > > > +	list.scalar("y");
> > > > > +   \endcode
> > > > > + *
> > > > > + * and with dictionaries and lists too
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict subDict = list.dict();
> > > > > +	YamlList subList = list.list();
> > > > > +   \endcode
> > > >
> > > > The biggest issue with the API is that it can be easily misused:
> > > >
> > > > 	YamlDict list1 = dict.list("foo");
> > > > 	YamlDict list2 = dict.list("bar");
> > > > 	list1.scalar("0");
> > > >
> > > > I'm still annoyed that I haven't found a neat way to prevent this. I
> > > > wonder if we could make it a bit safer by at least catching incorrect
> > > > usage at runtime. What I'm thinking is
> > >
> > > Yes, unfortunately I don't think we can easily catch this at build
> > > time
> > >
> > > > - When creating a child, recording the child pointer in the parent, and
> > > >   the parent pointer in the child
> > > > - If the parent already has a child when a new child is created, reset
> > > >   the parent pointer of the previous child. This makes the previous
> > > >   child invalid.
> > > > - Any invalidation of a child needs to be propagated to its children.
> > > > - Any operation on an invalid child would be caught and logged.
> > > > - When a child is destroyed, if its parent is not null, reset the child
> > > >   pointer in the parent to null.
> > > > - If a parent is destroyed while it has a child pointer, reset the
> > > >   child's parent pointer to null and log an error.
> > > >
> > >
> > > That should be enough, I wonder if we can make access to an invalid
> > > childer a compiler error...
> >
> > I'd love to, but haven't found a nice way to do so :-S
> > [...]
>
> My guess is that the C++ type system is "too weak" in this sense, to implement
> the kind of checking you envision.
>
> If one is willing to trade indentation for a smaller chance of misuse, then one could:
> (Or at least I think such an API where the structure of the resulting YAML file
>  is in a sense encapsulated in the layout of the source code leads to fewer misuses.)
>
>    YamlDict root;
>
>    root.dict("foo", [](auto& d) {
>      d.list("x", [](auto& l) {
>        l.scalar(1);
>        l.scalar(true);
>      });
>      d.scalar("y", 42);
>    });
>
>    root.list("bar", [](auto&) {
>      ...
>    });
>
>    /* the methods could return an appropriate reference to make some kind of chaining possible: */
>
>    YamlDict()
>      .dict("foo", [](auto& d) {
>         d
>           .list("x", [](auto& l) {
>              l.scalar(1)
>               .scalar(true);
>           })
>           .scalar("y", 42);
>       })
>      .list("bar", [](auto&) { ... });
>
> Or one could even go fully in on some kind of chaining:
>
> YamlDict()
> 	.dict("foo")
> 		.list("x")
> 			.scalar(1)
> 			.scalar(true)
> 		.finish() /* end list */
> 		.scalar()
> 	.finish() /* end dict */
> 	.list("bar")
> 		...
>
>
> I am not sure if this has already been considered, sorry if it has been.

It hasn't, thanks for the input.

The problem I see, as you can see in the next patches, is that we need
to emit yaml, in example, every frame, programmatically. I don't think
the above can be constructed iteratively, could it ?

>
> Regards,
> Barnabás Pőcze

Hi


2024. november 29., péntek 19:24 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Fri, Nov 29, 2024 at 05:28:01PM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2024. november 29., péntek 09:44 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:
> >
> > > On Fri, Nov 29, 2024 at 09:35:06AM +0100, Jacopo Mondi wrote:
> > > > On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> > > > > On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > > > > > Implement a helpers to allow outputting text in YAML format.
> > > > > [...]
> > > > > > + *
> > > > > > + * To emit YAML users of the these helper classes create a root node with
> > > > > > + *
> > > > > > + * \code
> > > > > > +	std::string filePath("...");
> > > > > > +	auto root = YamlEmitter::root(filePath);
> > > > > > +   \endcode
> > > > > > + *
> > > > > > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > > > > > + * and YamlRoot::list() functions.
> > > > >
> > > > > I would write "and start emitting dictionaries and lists", "populating"
> > > > > sounds more like a YAML writer.
> > > > >
> > > > ditto
> > > >
> > > > > > + *
> > > > > > + * The classes part of this file implement RAII-style handling of YAML
> > > > > > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > > > > > + * sequence start and mapping start events are emitted and once the instances
> > > > > > + * gets destroyed the corresponding sequence end and mapping end events are
> > > > > > + * emitted.
> > > > > > + *
> > > > > > + * From an initialized YamlRoot instance is possible to create YAML list and
> > > > > > + * dictionaries.
> > > > > > + *
> > > > > > + * \code
> > > > > > +	YamlDict dict = root.dict();
> > > > > > +	YamlList list = root.list();
> > > > > > +   \endcode
> > > > > > + *
> > > > > > + * YamlDict instances can be populated with scalars associated with a key
> > > > > > + *
> > > > > > + * \code
> > > > > > +	dict["key"] = "value";
> > > > > > +   \endcode
> > > > > > + *
> > > > > > + * and it is possible to create lists and dictionaries, associated with a key
> > > > > > + *
> > > > > > + * \code
> > > > > > +	YamlDict subDict = dict.dict("newDict");
> > > > > > +	YamlList subList = dict.list("newList");
> > > > > > +   \endcode
> > > > > > + *
> > > > > > + * YamlList instances can be populated with scalar elements
> > > > > > + *
> > > > > > + * \code
> > > > > > +	list.scalar("x");
> > > > > > +	list.scalar("y");
> > > > > > +   \endcode
> > > > > > + *
> > > > > > + * and with dictionaries and lists too
> > > > > > + *
> > > > > > + * \code
> > > > > > +	YamlDict subDict = list.dict();
> > > > > > +	YamlList subList = list.list();
> > > > > > +   \endcode
> > > > >
> > > > > The biggest issue with the API is that it can be easily misused:
> > > > >
> > > > > 	YamlDict list1 = dict.list("foo");
> > > > > 	YamlDict list2 = dict.list("bar");
> > > > > 	list1.scalar("0");
> > > > >
> > > > > I'm still annoyed that I haven't found a neat way to prevent this. I
> > > > > wonder if we could make it a bit safer by at least catching incorrect
> > > > > usage at runtime. What I'm thinking is
> > > >
> > > > Yes, unfortunately I don't think we can easily catch this at build
> > > > time
> > > >
> > > > > - When creating a child, recording the child pointer in the parent, and
> > > > >   the parent pointer in the child
> > > > > - If the parent already has a child when a new child is created, reset
> > > > >   the parent pointer of the previous child. This makes the previous
> > > > >   child invalid.
> > > > > - Any invalidation of a child needs to be propagated to its children.
> > > > > - Any operation on an invalid child would be caught and logged.
> > > > > - When a child is destroyed, if its parent is not null, reset the child
> > > > >   pointer in the parent to null.
> > > > > - If a parent is destroyed while it has a child pointer, reset the
> > > > >   child's parent pointer to null and log an error.
> > > > >
> > > >
> > > > That should be enough, I wonder if we can make access to an invalid
> > > > childer a compiler error...
> > >
> > > I'd love to, but haven't found a nice way to do so :-S
> > > [...]
> >
> > My guess is that the C++ type system is "too weak" in this sense, to implement
> > the kind of checking you envision.
> >
> > If one is willing to trade indentation for a smaller chance of misuse, then one could:
> > (Or at least I think such an API where the structure of the resulting YAML file
> >  is in a sense encapsulated in the layout of the source code leads to fewer misuses.)
> >
> >    YamlDict root;
> >
> >    root.dict("foo", [](auto& d) {
> >      d.list("x", [](auto& l) {
> >        l.scalar(1);
> >        l.scalar(true);
> >      });
> >      d.scalar("y", 42);
> >    });
> >
> >    root.list("bar", [](auto&) {
> >      ...
> >    });
> >
> >    /* the methods could return an appropriate reference to make some kind of chaining possible: */
> >
> >    YamlDict()
> >      .dict("foo", [](auto& d) {
> >         d
> >           .list("x", [](auto& l) {
> >              l.scalar(1)
> >               .scalar(true);
> >           })
> >           .scalar("y", 42);
> >       })
> >      .list("bar", [](auto&) { ... });
> >
> > Or one could even go fully in on some kind of chaining:
> >
> > YamlDict()
> > 	.dict("foo")
> > 		.list("x")
> > 			.scalar(1)
> > 			.scalar(true)
> > 		.finish() /* end list */
> > 		.scalar()
> > 	.finish() /* end dict */
> > 	.list("bar")
> > 		...
> >
> >
> > I am not sure if this has already been considered, sorry if it has been.
> 
> It hasn't, thanks for the input.
> 
> The problem I see, as you can see in the next patches, is that we need
> to emit yaml, in example, every frame, programmatically. I don't think
> the above can be constructed iteratively, could it ?
> [...]

Ahh, sorry, you're right. One item is added to a list in every call of `dumpRequest()`.
In this case there does not seem to be a good way to use the type system to
enforce much of anything without runtime checking.

YAML allows multiple documents in a stream, so theoretically every request could
be described by its own separate document, which would avoid the need for keeping
unterminated YAML structures around between calls. But I am not sure what kind
of challenges that would bring (especially on the parsing side), and if it is
worth it.


Regards,
Barnabás Pőcze

On Fri, Nov 29, 2024 at 05:28:01PM +0000, Barnabás Pőcze wrote:
> 2024. november 29., péntek 09:44 keltezéssel, Laurent Pinchart írta:
> > On Fri, Nov 29, 2024 at 09:35:06AM +0100, Jacopo Mondi wrote:
> > > On Wed, Nov 27, 2024 at 08:04:06AM +0200, Laurent Pinchart wrote:
> > > > On Wed, Nov 06, 2024 at 06:58:51PM +0100, Jacopo Mondi wrote:
> > > > > Implement a helpers to allow outputting text in YAML format.
> > > > [...]
> > > > > + *
> > > > > + * To emit YAML users of the these helper classes create a root node with
> > > > > + *
> > > > > + * \code
> > > > > +	std::string filePath("...");
> > > > > +	auto root = YamlEmitter::root(filePath);
> > > > > +   \endcode
> > > > > + *
> > > > > + * and start populating it with dictionaries and lists with the YamlRoot::dict()
> > > > > + * and YamlRoot::list() functions.
> > > >
> > > > I would write "and start emitting dictionaries and lists", "populating"
> > > > sounds more like a YAML writer.
> > > >
> > > ditto
> > >
> > > > > + *
> > > > > + * The classes part of this file implement RAII-style handling of YAML
> > > > > + * events. By creating a YamlList and YamlDict instance the associated YAML
> > > > > + * sequence start and mapping start events are emitted and once the instances
> > > > > + * gets destroyed the corresponding sequence end and mapping end events are
> > > > > + * emitted.
> > > > > + *
> > > > > + * From an initialized YamlRoot instance is possible to create YAML list and
> > > > > + * dictionaries.
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict dict = root.dict();
> > > > > +	YamlList list = root.list();
> > > > > +   \endcode
> > > > > + *
> > > > > + * YamlDict instances can be populated with scalars associated with a key
> > > > > + *
> > > > > + * \code
> > > > > +	dict["key"] = "value";
> > > > > +   \endcode
> > > > > + *
> > > > > + * and it is possible to create lists and dictionaries, associated with a key
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict subDict = dict.dict("newDict");
> > > > > +	YamlList subList = dict.list("newList");
> > > > > +   \endcode
> > > > > + *
> > > > > + * YamlList instances can be populated with scalar elements
> > > > > + *
> > > > > + * \code
> > > > > +	list.scalar("x");
> > > > > +	list.scalar("y");
> > > > > +   \endcode
> > > > > + *
> > > > > + * and with dictionaries and lists too
> > > > > + *
> > > > > + * \code
> > > > > +	YamlDict subDict = list.dict();
> > > > > +	YamlList subList = list.list();
> > > > > +   \endcode
> > > >
> > > > The biggest issue with the API is that it can be easily misused:
> > > >
> > > > 	YamlDict list1 = dict.list("foo");
> > > > 	YamlDict list2 = dict.list("bar");
> > > > 	list1.scalar("0");
> > > >
> > > > I'm still annoyed that I haven't found a neat way to prevent this. I
> > > > wonder if we could make it a bit safer by at least catching incorrect
> > > > usage at runtime. What I'm thinking is
> > >
> > > Yes, unfortunately I don't think we can easily catch this at build
> > > time
> > >
> > > > - When creating a child, recording the child pointer in the parent, and
> > > >   the parent pointer in the child
> > > > - If the parent already has a child when a new child is created, reset
> > > >   the parent pointer of the previous child. This makes the previous
> > > >   child invalid.
> > > > - Any invalidation of a child needs to be propagated to its children.
> > > > - Any operation on an invalid child would be caught and logged.
> > > > - When a child is destroyed, if its parent is not null, reset the child
> > > >   pointer in the parent to null.
> > > > - If a parent is destroyed while it has a child pointer, reset the
> > > >   child's parent pointer to null and log an error.
> > > >
> > >
> > > That should be enough, I wonder if we can make access to an invalid
> > > childer a compiler error...
> > 
> > I'd love to, but haven't found a nice way to do so :-S
> > [...]
> 
> My guess is that the C++ type system is "too weak" in this sense, to implement
> the kind of checking you envision.

We would need a way, when creating and destroying child objects, to
alter the state of the parent object in a way that could be checked at
compile time. I don't think that's doable indeed just with the C++
language (and I won't explore the option of developing a compiler
plugin).

> If one is willing to trade indentation for a smaller chance of misuse, then one could:
> (Or at least I think such an API where the structure of the resulting YAML file
>  is in a sense encapsulated in the layout of the source code leads to fewer misuses.)
> 
>    YamlDict root;
> 
>    root.dict("foo", [](auto& d) {
>      d.list("x", [](auto& l) {
>        l.scalar(1);
>        l.scalar(true);
>      });
>      d.scalar("y", 42);
>    });
> 
>    root.list("bar", [](auto&) {
>      ...
>    });
> 
>    /* the methods could return an appropriate reference to make some kind of chaining possible: */
>    
>    YamlDict()
>      .dict("foo", [](auto& d) {
>         d
>           .list("x", [](auto& l) {
>              l.scalar(1)
>               .scalar(true);
>           })
>           .scalar("y", 42);
>       })
>      .list("bar", [](auto&) { ... });
> 
> Or one could even go fully in on some kind of chaining:
> 
> YamlDict()
> 	.dict("foo")
> 		.list("x")
> 			.scalar(1)
> 			.scalar(true)
> 		.finish() /* end list */
> 		.scalar()
> 	.finish() /* end dict */
> 	.list("bar")
> 		...
> 
> 
> I am not sure if this has already been considered, sorry if it has been.