[libcamera-devel,2/7] libcamera: yaml_parser: Add iterator API
diff mbox series

Message ID 20220616142403.20723-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: yaml_parser: Add iterator API
Related show

Commit Message

Laurent Pinchart June 16, 2022, 2:23 p.m. UTC
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@ideasonboard.com>
---
 include/libcamera/internal/yaml_parser.h | 117 ++++++++++++++++++++++-
 src/libcamera/yaml_parser.cpp            |  39 ++++++++
 2 files changed, 154 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi June 17, 2022, 1:45 p.m. UTC | #1
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@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
>
Laurent Pinchart June 17, 2022, 2:09 p.m. UTC | #2
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@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
Jacopo Mondi June 17, 2022, 2:20 p.m. UTC | #3
Hi Laurent

  thanks for the clarifications, with the minor issue of the iterator
type changed

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
     j

On Fri, Jun 17, 2022 at 05:09:16PM +0300, Laurent Pinchart wrote:
> 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@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
Hanlin Chen June 20, 2022, 12:02 p.m. UTC | #4
Hi Laurent,

Reviewed-by: Han-Lin Chen <hanlinchen@chromium.org>

Thanks for the extension. Looks great to me.
Just to confirm, I suppose the adding an adaptor instead of returning
the const version of map and list is to hide the underlying types?

On Fri, Jun 17, 2022 at 10:21 PM Jacopo Mondi via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Laurent
>
>   thanks for the clarifications, with the minor issue of the iterator
> type changed
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>      j
>
> On Fri, Jun 17, 2022 at 05:09:16PM +0300, Laurent Pinchart wrote:
> > 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@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
Laurent Pinchart June 20, 2022, 12:09 p.m. UTC | #5
Hi Han-Lin,

On Mon, Jun 20, 2022 at 08:02:13PM +0800, Hanlin Chen wrote:
> Hi Laurent,
> 
> Reviewed-by: Han-Lin Chen <hanlinchen@chromium.org>
> 
> Thanks for the extension. Looks great to me.
> Just to confirm, I suppose the adding an adaptor instead of returning
> the const version of map and list is to hide the underlying types?

That's right, it's mostly to hide the unique_ptr (and of course also
because libcamera wasn't making use of
https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern yet
:-)).

> On Fri, Jun 17, 2022 at 10:21 PM Jacopo Mondi via libcamera-devel wrote:
> >
> > Hi Laurent
> >
> >   thanks for the clarifications, with the minor issue of the iterator
> > type changed
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > On Fri, Jun 17, 2022 at 05:09:16PM +0300, Laurent Pinchart wrote:
> > > 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@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

Patch
diff mbox series

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;
+
+		Iterator(typename Container::const_iterator it)
+			: it_(it)
+		{
+		}
+
+		Derived &operator++()
+		{
+			++it_;
+			return *static_cast<Derived *>(this);
+		}
+
+		Derived operator++(int)
+		{
+			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_ }; }
+
 	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