[libcamera-devel] [PATCH 2/7] libcamera: yaml_parser: Add iterator API

[Jacopo Mondi]

Hi Laurent,

On Thu, Jun 16, 2022 at 05:23:58PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Allow using range-based for loops over YamlObject instances by
> implementing iterators. New YamlObject::DictAdapter and
> YamlObject::ListAdapter adapter classes are introduced to provide
> different iterators depending on the object type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/yaml_parser.h | 117 ++++++++++++++++++++++-
>  src/libcamera/yaml_parser.cpp            |  39 ++++++++
>  2 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 29b7b218f0d9..8edd32354184 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -7,6 +7,7 @@
>
>  #pragma once
>
> +#include <iterator>
>  #include <map>
>  #include <string>
>  #include <vector>
> @@ -22,7 +23,116 @@ class YamlParserContext;
>
>  class YamlObject
>  {
> +private:
> +	using DictContainer = std::map<std::string, std::unique_ptr<YamlObject>>;
> +	using ListContainer = std::vector<std::unique_ptr<YamlObject>>;
> +
>  public:
> +#ifndef __DOXYGEN__
> +	template<typename Container, typename Derived>
> +	class Iterator
> +	{
> +	public:
> +		using difference_type = std::ptrdiff_t;
> +		using iterator_category = std::bidirectional_iterator_tag;

Does it make any different the fact you only implement operator++ ?
Should this be just a forward_iterator ?

> +
> +		Iterator(typename Container::const_iterator it)
> +			: it_(it)
> +		{
> +		}
> +
> +		Derived &operator++()

Being it_ a ::const_iterator, should the returned type be a const too  ?

> +		{
> +			++it_;
> +			return *static_cast<Derived *>(this);

Weird, I was expecting this to be
			return *static_cast<Derived *>(it);

But I've surely missed something as the class clearly works :)

> +		}
> +
> +		Derived operator++(int)

Is the 'int' tag to allow random access ?

> +		{
> +			Derived it = *static_cast<Derived *>(this);
> +			it_++;
> +			return it;
> +		}
> +
> +		friend bool operator==(const Iterator &a, const Iterator &b)
> +		{
> +			return a.it_ == b.it_;
> +		}
> +
> +		friend bool operator!=(const Iterator &a, const Iterator &b)
> +		{
> +			return a.it_ != b.it_;
> +		}
> +
> +	protected:
> +		typename Container::const_iterator it_;
> +	};
> +
> +	template<typename Container, typename Iterator>
> +	class Adapter
> +	{
> +	public:
> +		Adapter(const Container &container)
> +			: container_(container)
> +		{
> +		}
> +
> +		Iterator begin() const
> +		{
> +			return Iterator{ container_.begin() };
> +		}
> +
> +		Iterator end() const
> +		{
> +			return Iterator{ container_.end() };
> +		}
> +
> +	protected:
> +		const Container &container_;
> +	};
> +
> +	class ListIterator : public Iterator<ListContainer, ListIterator>
> +	{
> +	public:
> +		using value_type = const YamlObject &;
> +		using pointer = const YamlObject *;
> +		using reference = value_type;
> +
> +		value_type operator*() const
> +		{
> +			return *it_->get();
> +		}
> +
> +		pointer operator->() const
> +		{
> +			return it_->get();
> +		}
> +	};
> +
> +	class DictIterator : public Iterator<DictContainer, DictIterator>
> +	{
> +	public:
> +		using value_type = std::pair<const std::string &, const YamlObject &>;
> +		using pointer = value_type *;
> +		using reference = value_type &;
> +
> +		value_type operator*() const
> +		{
> +			return { it_->first, *it_->second.get() };
> +		}
> +	};
> +
> +	class DictAdapter : public Adapter<DictContainer, DictIterator>
> +	{
> +	public:
> +		using key_type = std::string;
> +	};
> +
> +	class ListAdapter : public Adapter<ListContainer, ListIterator>
> +	{
> +	};
> +#endif /* __DOXYGEN__ */
> +
>  	YamlObject();
>  	~YamlObject();
>
> @@ -55,6 +165,9 @@ public:
>  #endif
>  	T get(const T &defaultValue, bool *ok = nullptr) const;
>
> +	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> +	ListAdapter asList() const { return ListAdapter{ list_ }; }

When I first thought about making this iterable I was expecting
simple wrappers around std::list iterator functions. That would not
allow to have it as dict or as list. I wonder if providing both
abstraction is necessary (my bad, I never played very much with this
class).

More questions than comments, so I would say the patch looks good (as
far as I understand it :p )

Thanks
   j

> +
>  	const YamlObject &operator[](std::size_t index) const;
>
>  	bool contains(const std::string &key) const;
> @@ -75,8 +188,8 @@ private:
>  	Type type_;
>
>  	std::string value_;
> -	std::vector<std::unique_ptr<YamlObject>> list_;
> -	std::map<const std::string, std::unique_ptr<YamlObject>> dictionary_;
> +	ListContainer list_;
> +	DictContainer dictionary_;
>  };
>
>  class YamlParser final
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 85f6694f5fde..4df7e5a33d47 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -259,6 +259,45 @@ Size YamlObject::get(const Size &defaultValue, bool *ok) const
>
>  #endif /* __DOXYGEN__ */
>
> +/**
> + * \fn YamlObject::asDict() const
> + * \brief Wrap a dictionary YamlObject in an adapter that exposes iterators
> + *
> + * The YamlObject class doesn't directly implement iterators, as the iterator
> + * type depends on whether the object is a Dictionary or List. This function
> + * wraps a YamlObject of Dictionary type into an adapter that exposes
> + * iterators, as well as begin() and end() functions, allowing usage of
> + * range-based for loops with YamlObject. As YAML mappings are not ordered, the
> + * iteration order is not specified.
> + *
> + * The iterator's value_type is a
> + * <em>std::pair<const std::string &, const \ref YamlObject &></em>.
> + *
> + * If the YamlObject is not of Dictionary type, the returned adapter operates
> + * as an empty container.
> + *
> + * \return An adapter of unspecified type compatible with range-based for loops
> + */
> +
> +/**
> + * \fn YamlObject::asList() const
> + * \brief Wrap a list YamlObject in an adapter that exposes iterators
> + *
> + * The YamlObject class doesn't directly implement iterators, as the iterator
> + * type depends on whether the object is a Dictionary or List. This function
> + * wraps a YamlObject of List type into an adapter that exposes iterators, as
> + * well as begin() and end() functions, allowing usage of range-based for loops
> + * with YamlObject. As YAML lists are ordered, the iteration order is identical
> + * to the list order in the YAML data.
> + *
> + * The iterator's value_type is a <em>const YamlObject &</em>.
> + *
> + * If the YamlObject is not of List type, the returned adapter operates as an
> + * empty container.
> + *
> + * \return An adapter of unspecified type compatible with range-based for loops
> + */
> +
>  /**
>   * \fn YamlObject::operator[](std::size_t index) const
>   * \brief Retrieve the element from list YamlObject by index
> --
> Regards,
>
> Laurent Pinchart
>