[libcamera-devel] [PATCH] libcamera: span: Make constructors explicit as required by C++20

[Jacopo Mondi]

Hi Laurent,

On Sat, Oct 24, 2020 at 08:12:25PM +0300, Laurent Pinchart wrote:
> The C++20 std::span class, after which Span is modelled, specifies four
> constructors as explicit when extent is not dynamic_extent. Align our
> implementation with those requirements.
>
> A careful reviewer may notice that this change addresses five
> constructors, not four. The reason is that the two constructors taking
> Container and const Container parameters are not specified in C++20,
> which uses a single constructor taking a range parameter instead. As
> ranges are not available in C++17, the Container constructors are our
> best effort at providing a similar feature.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/span.h | 62 ++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/span.h b/include/libcamera/span.h
> index c0e439923933..b0dec77c6b1b 100644
> --- a/include/libcamera/span.h
> +++ b/include/libcamera/span.h
> @@ -113,12 +113,12 @@ public:
>  	{
>  	}
>
> -	constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
> +	explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
>  		: data_(ptr)
>  	{
>  	}
>
> -	constexpr Span(pointer first, [[maybe_unused]] pointer last)
> +	explicit constexpr Span(pointer first, [[maybe_unused]] pointer last)
>  		: data_(first)
>  	{
>  	}
> @@ -154,7 +154,7 @@ public:
>  	}
>
>  	template<class Container>
> -	constexpr Span(Container &cont,
> +	explicit constexpr Span(Container &cont,
>  		       std::enable_if_t<!details::is_span<Container>::value &&

Does indentation need to be adjusted ?

>  					!details::is_array<Container>::value &&
>  					!std::is_array<Container>::value &&
> @@ -166,23 +166,23 @@ public:
>  	}
>
>  	template<class Container>
> -	constexpr Span(const Container &cont,
> -		       std::enable_if_t<!details::is_span<Container>::value &&
> -					!details::is_array<Container>::value &&
> -					!std::is_array<Container>::value &&
> -					std::is_convertible<std::remove_pointer_t<decltype(utils::data(cont))> (*)[],
> -							    element_type (*)[]>::value,
> -					std::nullptr_t> = nullptr)
> +	explicit constexpr Span(const Container &cont,
> +				std::enable_if_t<!details::is_span<Container>::value &&
> +						 !details::is_array<Container>::value &&
> +						 !std::is_array<Container>::value &&
> +						 std::is_convertible<std::remove_pointer_t<decltype(utils::data(cont))> (*)[],
> +								     element_type (*)[]>::value,
> +						 std::nullptr_t> = nullptr)
>  		: data_(utils::data(cont))
>  	{
>  		static_assert(utils::size(cont) == Extent, "Size mismatch");
>  	}
>
>  	template<class U, std::size_t N>
> -	constexpr Span(const Span<U, N> &s,
> -		       std::enable_if_t<std::is_convertible<U (*)[], element_type (*)[]>::value &&
> -					N == Extent,
> -					std::nullptr_t> = nullptr) noexcept
> +	explicit constexpr Span(const Span<U, N> &s,
> +				std::enable_if_t<std::is_convertible<U (*)[], element_type (*)[]>::value &&
> +						 N == Extent,
> +						 std::nullptr_t> = nullptr) noexcept
>  		: data_(s.data())
>  	{
>  	}
> @@ -212,24 +212,24 @@ public:
>  	constexpr Span<element_type, Count> first() const
>  	{
>  		static_assert(Count <= Extent, "Count larger than size");
> -		return { data(), Count };
> +		return Span<element_type, Count>{ data(), Count };
>  	}
>
>  	constexpr Span<element_type, dynamic_extent> first(std::size_t Count) const
>  	{
> -		return { data(), Count };
> +		return Span<element_type, dynamic_extent>{ data(), Count };
>  	}
>
>  	template<std::size_t Count>
>  	constexpr Span<element_type, Count> last() const
>  	{
>  		static_assert(Count <= Extent, "Count larger than size");
> -		return { data() + size() - Count, Count };
> +		return Span<element_type, Count>{ data() + size() - Count, Count };
>  	}
>
>  	constexpr Span<element_type, dynamic_extent> last(std::size_t Count) const
>  	{
> -		return { data() + size() - Count, Count };
> +		return Span<element_type, dynamic_extent>{ data() + size() - Count, Count };
>  	}
>
>  	template<std::size_t Offset, std::size_t Count = dynamic_extent>
> @@ -238,13 +238,19 @@ public:
>  		static_assert(Offset <= Extent, "Offset larger than size");
>  		static_assert(Count == dynamic_extent || Count + Offset <= Extent,
>  			      "Offset + Count larger than size");
> -		return { data() + Offset, Count == dynamic_extent ? size() - Offset : Count };
> +		return Span<element_type, Count != dynamic_extent ? Count : Extent - Offset>{
> +			data() + Offset,
> +			Count == dynamic_extent ? size() - Offset : Count
> +		};

Templates arguments specified with ternary operator.. C++ is amazing

>  	}
>
>  	constexpr Span<element_type, dynamic_extent>
>  	subspan(std::size_t Offset, std::size_t Count = dynamic_extent) const
>  	{
> -		return { data() + Offset, Count == dynamic_extent ? size() - Offset : Count };
> +		return Span<element_type, dynamic_extent>{
> +			data() + Offset,
> +			Count == dynamic_extent ? size() - Offset : Count
> +		};
>  	}
>
>  private:
> @@ -371,7 +377,7 @@ public:
>  	template<std::size_t Count>
>  	constexpr Span<element_type, Count> first() const
>  	{
> -		return { data(), Count };
> +		return Span<element_type, Count>{ data(), Count };
>  	}
>
>  	constexpr Span<element_type, dynamic_extent> first(std::size_t Count) const
> @@ -382,24 +388,30 @@ public:
>  	template<std::size_t Count>
>  	constexpr Span<element_type, Count> last() const
>  	{
> -		return { data() + size() - Count, Count };
> +		return Span<element_type, Count>{ data() + size() - Count, Count };
>  	}
>
>  	constexpr Span<element_type, dynamic_extent> last(std::size_t Count) const
>  	{
> -		return { data() + size() - Count, Count };
> +		return Span<element_type, dynamic_extent>{ data() + size() - Count, Count };
>  	}
>
>  	template<std::size_t Offset, std::size_t Count = dynamic_extent>
>  	constexpr Span<element_type, Count> subspan() const
>  	{
> -		return { data() + Offset, Count == dynamic_extent ? size() - Offset : Count };
> +		return Span<element_type, Count>{
> +			data() + Offset,
> +			Count == dynamic_extent ? size() - Offset : Count
> +		};
>  	}
>
>  	constexpr Span<element_type, dynamic_extent>
>  	subspan(std::size_t Offset, std::size_t Count = dynamic_extent) const
>  	{
> -		return { data() + Offset, Count == dynamic_extent ? size() - Offset : Count };
> +		return Span<element_type, dynamic_extent>{
> +			data() + Offset,
> +			Count == dynamic_extent ? size() - Offset : Count
> +		};

Looks good!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  	}
>
>  private:
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel