| Message ID | 20201024171225.14483-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-10-24 20:12:25 +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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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 && > !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 > + }; > } > > 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 > + }; > } > > private: > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@jmondi.org> Thanks j > } > > private: > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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 && !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 + }; } 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 + }; } private:
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@ideasonboard.com> --- include/libcamera/span.h | 62 ++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 25 deletions(-)