[libcamera-devel] [PATCH 2/7] libcamera: yaml_parser: Add iterator API
Hanlin Chen
hanlinchen at chromium.org
Mon Jun 20 14:02:13 CEST 2022
Hi Laurent,
Reviewed-by: Han-Lin Chen <hanlinchen at 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 at lists.libcamera.org> wrote:
>
> 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