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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jun 17, 2022 at 03:45:05PM +0200, Jacopo Mondi wrote:
> 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 ?

Very good point. I'll go for a forward_iterator as the goal is really to
support a for range loop. If someone wants to iterator backwards in the
future, this can then be extended.

> > +
> > +		Iterator(typename Container::const_iterator it)
> > +			: it_(it)
> > +		{
> > +		}
> > +
> > +		Derived &operator++()
> 
> Being it_ a ::const_iterator, should the returned type be a const too  ?

No, a const_iterator isn't const itself. The "const" is related to the
fact that the iterator doesn't allow modifying the element it points to,
but the iterator itself can be modified (incremented in this case).

> > +		{
> > +			++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 is the type of the class deriving from Iterator (DictIterator or
ListIterator), while it_ is the iterator on the underlying container.
The pre-increment operator returns a reference to itself, the only
reason why we need a cast is that DictIterator::operator++() should
return a DictIterator & and ListIterator::operator++() a ListIterator &.
As operator++() is implemented once in the base class only, we pass the
Derived type as a template argument, so the right type can be returned.

> > +		}
> > +
> > +		Derived operator++(int)
> 
> Is the 'int' tag to allow random access ?

It's used to differentiate the pre-increment (++x) and post-increment
(x++) operators, as C++ doesn't allow overloads to differ by they return type
only. It's a C++ convention, operator++() is the pre-increment operator,
and operator++(int) the post-increment operator.

> > +		{
> > +			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).

I would have liked a way to avoid asDict() and asList(). It's possible,
but then we would need the values to be stored in a single container
(done in the RFC v3 of the RPi YamlParser series), and the value_type of
the iterator to always be a pair, with the first member containing empty
strings when the YamlObject is a list. That's not very nice from an API
point of view.

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

Thank you :-)

> > +
> >  	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