[libcamera-devel] [PATCH v5 3/9] libcamera: yaml_parser: Add getList() function

Jacopo Mondi jacopo at jmondi.org
Thu Jul 28 12:39:02 CEST 2022 

Hi LAurent,

On Thu, Jul 28, 2022 at 01:27:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Jul 28, 2022 at 09:33:11AM +0200, Jacopo Mondi wrote:
> > On Thu, Jul 28, 2022 at 01:21:43AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > From: Florian Sylvestre <fsylvestre at baylibre.com>
> > >
> > > Allow to retrieve a YAML list of any already supported types in a
> > > std::vector.
> > >
> > > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > > Tested-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/yaml_parser.h | 16 +++++++
> > >  src/libcamera/yaml_parser.cpp            | 53 ++++++++++++++++++++++++
> > >  test/yaml-parser.cpp                     |  6 +++
> > >  3 files changed, 75 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > > index 61f2223223a7..4b3da0f03512 100644
> > > --- a/include/libcamera/internal/yaml_parser.h
> > > +++ b/include/libcamera/internal/yaml_parser.h
> > > @@ -174,6 +174,22 @@ public:
> > >  		return get<T>().value_or(defaultValue);
> > >  	}
> > >
> > > +#ifndef __DOXYGEN__
> > > +	template<typename T,
> > > +		 typename std::enable_if_t<
> >
> > nit: as you're using the enable_if_t helper, I guess the 'typename' keyword
> > can be dropped ? (gcc agrees)
>
> You're right. We have many occurrences of this through the code base,
> I'll fix them all on top of this series.
>
> > > +			 std::is_same_v<bool, T> ||
> > > +			 std::is_same_v<double, T> ||
> > > +			 std::is_same_v<int16_t, T> ||
> > > +			 std::is_same_v<uint16_t, T> ||
> > > +			 std::is_same_v<int32_t, T> ||
> > > +			 std::is_same_v<uint32_t, T> ||
> > > +			 std::is_same_v<std::string, T> ||
> > > +			 std::is_same_v<Size, T>> * = nullptr>
> > > +#else
> > > +	template<typename T>
> > > +#endif
> >
> > Why this #ifdef ? There are no overloads to confuse doxygen with,
> > right ?
>
> It's to avoid the enable_if appearing in the generated documentation.
> Otherwise it gets rendered as
>
> ◆ getList()
> template<typename T , typename std::enable_if_t< std::is_same_v< bool, T >||std::is_same_v< double, T >||std::is_same_v< int16_t, T >||std::is_same_v< uint16_t, T >||std::is_same_v< int32_t, T >||std::is_same_v< uint32_t, T >||std::is_same_v< std::string, T >||std::is_same_v< Size, T > > * = nullptr>
> template< typename T > libcamera::YamlObject::getList< T > ( ) const
>
> which isn't very nice. We do the same in other places, I can reconsider
> this, but I'd then like to do it globally.
>
> > > +	std::optional<std::vector<T>> getList() const;
> > > +
> > >  	DictAdapter asDict() const { return DictAdapter{ dictionary_ }; }
> > >  	ListAdapter asList() const { return ListAdapter{ list_ }; }
> > >
> > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > > index 4299f5abd38a..cb4f1eb29a77 100644
> > > --- a/src/libcamera/yaml_parser.cpp
> > > +++ b/src/libcamera/yaml_parser.cpp
> > > @@ -293,6 +293,59 @@ std::optional<Size> YamlObject::get() const
> > >
> > >  #endif /* __DOXYGEN__ */
> > >
> > > +/**
> > > + * \fn template<typename T> YamlObject::getList<T>() const
> > > + * \brief Parse the YamlObject as a list of \a T
> > > + *
> > > + * This function parses the value of the YamlObject as a list of \a T objects,
> > > + * and returns the value as a \a std::vector<T>. If parsing fails, std::nullopt
> > > + * is returned.
> > > + *
> > > + * \return The YamlObject value as a std::vector<T>, or std::nullopt if parsing
> > > + * failed
> > > + */
> > > +
> > > +#ifndef __DOXYGEN__
> > > +
> > > +template<typename T,
> > > +	 typename std::enable_if_t<
> > > +		 std::is_same_v<bool, T> ||
> > > +		 std::is_same_v<double, T> ||
> > > +		 std::is_same_v<int16_t, T> ||
> > > +		 std::is_same_v<uint16_t, T> ||
> > > +		 std::is_same_v<int32_t, T> ||
> > > +		 std::is_same_v<uint32_t, T> ||
> > > +		 std::is_same_v<std::string, T> ||
> > > +		 std::is_same_v<Size, T>> *>
> > > +std::optional<std::vector<T>> YamlObject::getList() const
> > > +{
> > > +	if (type_ != Type::List)
> > > +		return {};
> > > +
> > > +	std::vector<T> values;
> > > +	values.reserve(list_.size());
> > > +
> > > +	for (const YamlObject &entry : asList()) {
> > > +		const auto value = entry.get<T>();
> >
> > Should we be concerned about the type of value ?
>
> I'm not sure what you mean. Would you rather write
>
> 		const std::optional<T> value = entry.get<T>();
>
> or am I missing your point ?
>

I meant if we have to check if the type_ of value is of the expected
type :)

> > If not
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > +		if (!value)
> > > +			return {};
> > > +		values.emplace_back(*value);
> > > +	}
> > > +
> > > +	return values;
> > > +}
> > > +
> > > +template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
> > > +template std::optional<std::vector<double>> YamlObject::getList<double>() const;
> > > +template std::optional<std::vector<int16_t>> YamlObject::getList<int16_t>() const;
> > > +template std::optional<std::vector<uint16_t>> YamlObject::getList<uint16_t>() const;
> > > +template std::optional<std::vector<int32_t>> YamlObject::getList<int32_t>() const;
> > > +template std::optional<std::vector<uint32_t>> YamlObject::getList<uint32_t>() const;
> > > +template std::optional<std::vector<std::string>> YamlObject::getList<std::string>() const;
> > > +template std::optional<std::vector<Size>> YamlObject::getList<Size>() const;
> > > +
> > > +#endif /* __DOXYGEN__ */
> > > +
> > >  /**
> > >   * \fn YamlObject::asDict() const
> > >   * \brief Wrap a dictionary YamlObject in an adapter that exposes iterators
> > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > index ebb654f2ef9c..bb54e0d7f76a 100644
> > > --- a/test/yaml-parser.cpp
> > > +++ b/test/yaml-parser.cpp
> > > @@ -523,6 +523,12 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >
> > > +		const auto &values = firstElement.getList<uint16_t>();
> > > +		if (!values || values->size() != 2 || (*values)[0] != 1 || (*values)[1] != 2) {
> > > +			cerr << "getList() failed to return correct vector" << std::endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > >  		auto &secondElement = level2Obj[1];
> > >  		if (!secondElement.isDictionary() ||
> > >  		    !secondElement.contains("one") ||
>
> --
> Regards,
>
> Laurent Pinchart

More information about the libcamera-devel mailing list