[libcamera-devel] [PATCH 2/7] libcamera: yaml_parser: Add iterator API
Jacopo Mondi
jacopo at jmondi.org
Fri Jun 17 16:20:58 CEST 2022
Hi Laurent
thanks for the clarifications, with the minor issue of the iterator
type changed
Reviewed-by: Jacopo Mondi <jacopo at 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 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
More information about the libcamera-devel
mailing list