[libcamera-devel,v5,3/9] libcamera: yaml_parser: Add getList() function
diff mbox series

Message ID 20220727222149.30627-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • ipa: rkisp1: Add GSL, LSC, DPCC, Filter and CProc algorithms
Related show

Commit Message

Laurent Pinchart July 27, 2022, 10:21 p.m. UTC 
From: Florian Sylvestre <fsylvestre@baylibre.com>

Allow to retrieve a YAML list of any already supported types in a
std::vector.

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
Tested-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(+)

Comments

Jacopo Mondi July 28, 2022, 7:33 a.m. UTC | #1 
Hi Laurent,

On Thu, Jul 28, 2022 at 01:21:43AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
>
> Allow to retrieve a YAML list of any already supported types in a
> std::vector.
>
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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)

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

> +	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 ?
If not

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

Thanks
  j

> +		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
>
Laurent Pinchart July 28, 2022, 10:27 a.m. UTC | #2 
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@baylibre.com>
> >
> > Allow to retrieve a YAML list of any already supported types in a
> > std::vector.
> >
> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 ?

> If not
> 
> Reviewed-by: Jacopo Mondi <jacopo@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") ||
Jacopo Mondi July 28, 2022, 10:39 a.m. UTC | #3 
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@baylibre.com>
> > >
> > > Allow to retrieve a YAML list of any already supported types in a
> > > std::vector.
> > >
> > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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
Laurent Pinchart July 28, 2022, 10:40 a.m. UTC | #4 
Hi Jacopo,

On Thu, Jul 28, 2022 at 12:39:02PM +0200, Jacopo Mondi wrote:
> On Thu, Jul 28, 2022 at 01:27:37PM +0300, Laurent Pinchart wrote:
> > 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@baylibre.com>
> > > >
> > > > Allow to retrieve a YAML list of any already supported types in a
> > > > std::vector.
> > > >
> > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > > Tested-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 it's not of type T, get<T>() will return std::nullopt, so the !value
check below handles that.

> > > If not
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@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") ||

Patch
diff mbox series 

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<
+			 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
+	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>();
+		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") ||