[ PATCH ] C++20 <span>
Jonathan Wakely
jwakely@redhat.com
Tue Sep 3 13:31:00 GMT 2019
On 30/08/19 19:42 -0400, JeanHeyd Meneide wrote:
>Ahem -- we were supposed to use the 20 version of the constexpr macro,
>not the base one.
>I will note that, for some reason, the default constructor was already
>constexpr, so we don't change that one!
Thanks!
I've done a thorough review now, lots of comments below.
>On Fri, Aug 30, 2019 at 5:11 PM JeanHeyd Meneide
><phdofthehouse@gmail.com> wrote:
>>
>> On Fri, Aug 30, 2019 at 3:41 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>> >
>> > On 30/08/19 15:22 -0400, JeanHeyd Meneide wrote:
>> > >This patch implements <span> as it currently exists in the C++20 Working Draft.
>> >
>> > Nice!
>> >
>> >
>> > >Notes:
>> > >- __gnu_cxx::__normal_iterator is not fully constexpr, so its not used here
>> >
>> > I'd prefer to make __normal_iterator constexpr, and use it.
>> > It needs to be constexpr anyway for string and vector.
>> > ...
>>
>> Alright, thank you for the feedback. I fixed it up!
>>
>> Tested x86_64-pc-linux-gnu.
>>
>> 2019-08-30 JeanHeyd "ThePhD" Meneide <phdofthehouse@gmail.com>
>>
>> * include/std/span: Implement the entirety of span.
>> * include/bits/stl_iterator.h: __gnu_cxx::__normal_iterator<T,
>> C> is now constexpr-qualified for C++11+.
>> * include/bits/range_access.h: Add __adl_* versions of access functions.
>> * testsuite/23_containers/span/everything.cc: constexpr and
>> non-constexpr tests.
>> * include/Makefile.in: Add span to install.
>> * include/Makefile.am: Likewise
>diff --git a/.gitignore b/.gitignore
>index d9d3967a12c..fd116c362ac 100644
>--- a/.gitignore
>+++ b/.gitignore
>@@ -57,3 +57,7 @@ REVISION
> /mpc*
> /gmp*
> /isl*
>+
>+# ignore sprinkled in dev files that keep popping up
>+.vscode/
>+.vs/
No thanks :-)
If you want to add things to be ignored in your local repo then you
can put these entries in .git/info/exclude which is not checked in.
>diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
>index 3fe80f32cc4..b8b786d9260 100644
>--- a/libstdc++-v3/include/Makefile.am
>+++ b/libstdc++-v3/include/Makefile.am
>@@ -68,6 +68,7 @@ std_headers = \
> ${std_srcdir}/scoped_allocator \
> ${std_srcdir}/set \
> ${std_srcdir}/shared_mutex \
>+ ${std_srcdir}/span \
> ${std_srcdir}/sstream \
> ${std_srcdir}/stack \
> ${std_srcdir}/stdexcept \
>diff --git a/libstdc++-v3/include/Makefile.in b/libstdc++-v3/include/Makefile.in
>index b675d356cd4..cd1e9df5482 100644
>--- a/libstdc++-v3/include/Makefile.in
>+++ b/libstdc++-v3/include/Makefile.in
This file is generated by running autoreconf, so doesn't need to be
included in patches. Not a problem here, but for patches that makes
lots of changes to the configury stuff, it's just noise to include the
generated parts in the patch.
>@@ -412,6 +412,7 @@ std_headers = \
> ${std_srcdir}/scoped_allocator \
> ${std_srcdir}/set \
> ${std_srcdir}/shared_mutex \
>+ ${std_srcdir}/span \
> ${std_srcdir}/sstream \
> ${std_srcdir}/stack \
> ${std_srcdir}/stdexcept \
>diff --git a/libstdc++-v3/include/bits/range_access.h b/libstdc++-v3/include/bits/range_access.h
>index d1e74711433..3acaebadcf1 100644
>--- a/libstdc++-v3/include/bits/range_access.h
>+++ b/libstdc++-v3/include/bits/range_access.h
>@@ -318,6 +318,72 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> #endif // C++17
>
>+#if __cplusplus > 201703L
>+ // "why are these in namespace std:: and not __gnu_cxx:: ?"
>+ // because if we don't put them here it's impossible to
>+ // have implicit ADL with "using std::begin/end/size/data;".
>+ template <typename _Container>
>+ constexpr auto
>+ __adl_begin(_Container& __cont) noexcept(noexcept(begin(__cont)))
>+ { return begin(__cont); }
It looks like__adl_begin and __adl_end should be guarded by #ifdef
_GLIBCXX_P1394 because they're not needed otherwise.
>diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
>index 8ab0d72b0c2..f6a6060f2e3 100644
>--- a/libstdc++-v3/include/bits/stl_iterator.h
>+++ b/libstdc++-v3/include/bits/stl_iterator.h
>@@ -799,55 +799,58 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> typedef typename __traits_type::reference reference;
> typedef typename __traits_type::pointer pointer;
>
>+ // this was already constexpr
>+ // so it will not be changed to
>+ // _GLIBCXX_CONSTEXPR20
There's no need to add a comment saying what you didn't change :-)
> _GLIBCXX_CONSTEXPR __normal_iterator() _GLIBCXX_NOEXCEPT
> : _M_current(_Iterator()) { }
>
> explicit
>- __normal_iterator(const _Iterator& __i) _GLIBCXX_NOEXCEPT
>+ _GLIBCXX_CONSTEXPR20 __normal_iterator(const _Iterator& __i) _GLIBCXX_NOEXCEPT
The correct macro is _GLIBCXX20_CONSTEXPR. Was this patch tested?
Please put it on the previous line with 'explicit' i.e.
explicit _GLIBCXX20_CONSTEXPR
__normal_iterator(...
> template<typename _Iterator, typename _Container>
>- inline bool
>+ _GLIBCXX_CONSTEXPR20 inline bool
> operator>=(const __normal_iterator<_Iterator, _Container>& __lhs,
> const __normal_iterator<_Iterator, _Container>& __rhs)
> _GLIBCXX_NOEXCEPT
>@@ -973,26 +976,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _IteratorL, typename _IteratorR, typename _Container>
> #if __cplusplus >= 201103L
> // DR 685.
>- inline auto
>+ _GLIBCXX_CONSTEXPR20 inline auto
> operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
> const __normal_iterator<_IteratorR, _Container>& __rhs) noexcept
> -> decltype(__lhs.base() - __rhs.base())
> #else
>- inline typename __normal_iterator<_IteratorL, _Container>::difference_type
>+ _GLIBCXX_CONSTEXPR20 inline typename __normal_iterator<_IteratorL, _Container>::difference_type
There's no need to put it here, because this #else branch is for C++98
and the macro will always expand to nothing for C++98.
>+// get on someone's back about it in Belfast!!!
>+#define __cpp_lib_span 201911
>+
>+ inline constexpr size_t dynamic_extent =
>+ static_cast<size_t>(-1);
>+
>+ namespace __detail
>+ {
>+
>+ template<typename _Element, typename _ToElement>
>+ using __is_base_derived_safe_convertible =
>+ is_convertible<_Element (*)[], _ToElement (*)[]>;
Do we need this alias template if the only place it's used is here ...
>+ template<typename _Element, typename _ToElement>
>+ static constexpr inline bool __is_base_derived_safe_convertible_v =
>+ __is_base_derived_safe_convertible<_Element, _ToElement>::value;
Could we just define the variable template in terms of
is_convertible_v?
>+ template<typename>
All your templates are indented wrong, there should be another level
of indentation after the template-head.
>+ struct __is_std_array : false_type
>+ {
>+ };
>+
>+ template<typename _Element, size_t _Extent>
>+ struct __is_std_array<::std::array<_Element, _Extent>> : true_type
This should be _GLIBCXX_STD_C::array because when _GLIBCXX_DEBUG is
defined std::array is the same type as std::__debug::array
(std::__debug is an inline namespace). That will mean the following
specialization a redefinition of the one above, which is an error.
>+ {
>+ };
>+
>+#ifdef _GLIBCXX_DEBUG
>+ template<typename _Element, size_t _Extent>
>+ struct __is_std_array<::std::__debug::array<_Element, _Extent>>
>+ : true_type
>+ {
>+ };
>+#endif // debug/array
>+
>+ template<typename _Type>
>+ inline constexpr bool __is_std_array_v = __is_std_array<_Type>::value;
If we don't need the __is_std_array class templates directly we could
just specialize this variable template instead.
template<typename _Tp>
inline constexpr bool __is_std_array_v = false;
template<typename _Tp, size_t _Num>
inline constexpr bool
__is_std_array_v<_GLIBCXX_STD_C::array<_Tp, _Num>> = true;
#ifdef _GLIBCXX_DEBUG
template<typename _Tp, size_t _Num>
inline constexpr bool
__is_std_array_v<std::__debug::array<_Tp, _Num>> = true;
#endif // debug/array
>+ template<size_t _Extent>
>+ struct __extent_storage
>+ {
>+
>+ constexpr __extent_storage() noexcept = default;
An empty line between these constructors please.
>+ constexpr __extent_storage(size_t) noexcept
>+ {
>+ }
And the closing brace can be on the same line as the opening one:
{ }
>+
>+ static constexpr size_t
>+ _M_extent() noexcept
>+ {
>+ return _Extent;
>+ }
Even if not empty, one line functions can be written like
{ return _Extent; }
>+ };
[...]
>+ template<typename _Type, size_t _Extent = dynamic_extent>
>+ class span : private __detail::__extent_storage<_Extent>
>+ {
>+ public:
>+ // member types
>+ using value_type = remove_cv_t<_Type>;
>+ using element_type = _Type;
>+ using index_type = size_t;
>+ using reference = element_type&;
>+ using const_reference = const element_type&;
>+ using pointer = _Type*;
>+ using const_pointer = const _Type*;
>+ using iterator = __gnu_cxx::__normal_iterator<pointer, span>;
>+ using const_iterator = __gnu_cxx::__normal_iterator<const_pointer, span>;
>+ using reverse_iterator = ::std::reverse_iterator<iterator>;
>+ using const_reverse_iterator = ::std::reverse_iterator<const_iterator>;
>+ using difference_type = ptrdiff_t;
>+ // Official wording has no size_type -- why??
>+ // using size_type = size_t;
>+
>+ // member constants
>+ static inline constexpr size_t extent = _Extent;
>+
>+ private:
>+ using __base_t = __detail::__extent_storage<_Extent>;
>+
>+ public:
>+ // constructors
>+ constexpr span() noexcept : __base_t(), _M_ptr(nullptr)
This constructor needs SFINAE constraints:
Constraints: Extent == dynamic_extent || Extent == 0 is true.
>+ {
>+ }
>+
>+ constexpr span(const span&) noexcept = default;
>+
>+ template<size_t _ArrayExtent,
>+ enable_if_t<
>+ (_Extent == dynamic_extent || _ArrayExtent == _Extent) &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(::std::__adl_data(
>+ ::std::declval<element_type (&)[_ArrayExtent]>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(element_type (&__arr)[_ArrayExtent]) noexcept(
>+ noexcept(::std::__adl_data(__arr)))
>+ : span(::std::__adl_data(__arr), _ArrayExtent)
>+ {
>+ }
>+
>+ template<size_t _ArrayExtent,
>+ enable_if_t<
>+ (_Extent == dynamic_extent || _ArrayExtent == _Extent) &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(::std::__adl_data(
>+ ::std::declval<array<value_type, _ArrayExtent>&>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(array<value_type, _ArrayExtent>& __arr) noexcept(
>+ noexcept(::std::__adl_data(__arr)))
>+ : span(::std::__adl_data(__arr), _ArrayExtent)
>+ {
>+ }
>+
>+ template<size_t _ArrayExtent,
>+ enable_if_t<
>+ (_Extent == dynamic_extent || _ArrayExtent == _Extent) &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(::std::__adl_data(
>+ ::std::declval<const array<value_type, _ArrayExtent>&>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(const array<value_type, _ArrayExtent>&
>+ __arr) noexcept(noexcept(::std::__adl_data(__arr)))
>+ : span(::std::__adl_data(__arr), _ArrayExtent)
>+ {
>+ }
>+
>+ // NOTE: when the time comes, and P1394 -
>+ // range constructors for std::span - ships in
>+ // the standard, delete the #else block and remove
>+ // the conditional
>+ // if the paper fails, delete #if block
>+ // and keep the crappy #else block
>+ // and then cry that NB comments failed C++20...
>+ // but maybe for C++23?
>+#if defined(_GLIBCXX_P1394) && _GLIBCXX_P1394
>+ template<typename _Range,
>+ enable_if_t<
>+ (_Extent == dynamic_extent) &&
>+ !is_same_v<remove_cvref_t<_Range>, span> &&
>+ !__detail::__is_std_array_v<remove_cvref_t<_Range>> &&
>+ !is_array_v<remove_cvref_t<_Range>> &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(
>+ ::std::__adl_data(::std::declval<_Range&>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(_Range&& __range) noexcept(
>+ noexcept(::std::__adl_data(__range)) &&
>+ noexcept(::std::__adl_size(__range)))
>+ : span(::std::__adl_data(__range), ::std::__adl_size(__range))
>+ {
>+ }
>+
>+ template<typename _ContiguousIterator, typename _Sentinel,
>+ enable_if_t<!is_convertible_v<_Sentinel, index_type> &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_reference_t<typename
>+ iterator_traits<_ContiguousIterator>::reference>,
>+ element_type>>* = nullptr>
>+ constexpr span(_ContiguousIterator __first, _Sentinel __last)
>+ : span(::std::move(__first), static_cast<index_type>(__last - __first))
>+ {
>+ }
>+
>+ template<typename _ContiguousIterator>
>+ constexpr span(_ContiguousIterator __first, index_type __count) noexcept(
>+ noexcept(::std::to_address(__first)))
>+ : __base_t(__count), _M_ptr(::std::to_address(__first))
>+ {
>+ }
>+#else
>+ template<typename _Container,
>+ enable_if_t<
>+ (_Extent == dynamic_extent) &&
>+ !is_same_v<remove_cvref_t<_Container>, span> &&
>+ !__detail::__is_std_array_v<remove_cvref_t<_Container>> &&
>+ !is_array_v<remove_cvref_t<_Container>> &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(
>+ ::std::__adl_data(::std::declval<_Container&>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(_Container& __range) noexcept(
>+ noexcept(::std::__adl_data(__range)) &&
>+ noexcept(::std::__adl_size(__range)))
>+ : span(::std::__adl_data(__range), ::std::__adl_size(__range))
>+ {
>+ }
>+
>+ template<typename _Container,
>+ enable_if_t<
>+ (_Extent == dynamic_extent) &&
>+ !is_same_v<remove_cvref_t<_Container>, span> &&
>+ !__detail::__is_std_array_v<remove_cvref_t<_Container>> &&
>+ !is_array_v<remove_cvref_t<_Container>> &&
>+ __detail::__is_base_derived_safe_convertible_v<
>+ remove_pointer_t<decltype(
>+ ::std::__adl_data(::std::declval<_Container&>()))>,
>+ element_type>>* = nullptr>
>+ constexpr span(const _Container& __range) noexcept(
>+ noexcept(::std::__adl_data(__range)) &&
>+ noexcept(::std::__adl_size(__range)))
>+ : span(::std::__adl_data(__range), ::std::__adl_size(__range))
>+ {
>+ }
>+
>+ constexpr span(pointer __first, pointer __last) noexcept
>+ : span(::std::move(__first), static_cast<index_type>(__last - __first))
>+ {
>+ }
Blank line between these ctors please. It would also be helpful to
order the ctors as shown in the draft.
>+ constexpr span(pointer __first, index_type __count) noexcept
>+ : __base_t(__count), _M_ptr(static_cast<pointer>(__first))
>+ {
This could use an assertion to check one of the "Expects:"
preconditions:
__glibcxx_assert(extent == dynamic_extent || __count == extent);
We can't check that [ptr, ptr+count) is a valid range, that's just
something users need to ensure.
>+ }
>+#endif // P1394
>+
>+ // assignment
>+ constexpr span&
>+ operator=(const span&) noexcept = default;
>+
>+ // observers
>+ constexpr reference
>+ front() noexcept
>+ {
>+ return *this->begin();
>+ }
This member function isn't shown in the draft.
>+
>+ constexpr const_reference
>+ front() const noexcept
>+ {
>+ return *this->begin();
>+ }
>+
>+ constexpr reference
>+ back() noexcept
>+ {
>+ return *(this->begin() + 1);
>+ }
Not shown in the draft.
>+ constexpr const_reference
>+ back() const noexcept
>+ {
>+ return *(this->end() - 1);
>+ }
>+
>+ constexpr reference operator[](index_type __idx) noexcept
>+ {
>+ return *(this->_M_ptr + __idx);
>+ }
Not shown in the draft.
>+ constexpr const_reference operator[](index_type __idx) const noexcept
>+ {
>+ return *(this->_M_ptr + __idx);
>+ }
>+
>+ constexpr pointer
>+ data() const noexcept
>+ {
>+ return this->_M_ptr;
>+ }
>+
>+ constexpr index_type
>+ size() const noexcept
>+ {
>+ return this->__base_t::_M_extent();
>+ }
>+
>+ constexpr index_type
>+ size_bytes() const noexcept
>+ {
>+ return this->__base_t::_M_extent() * sizeof(element_type);
>+ }
>+
>+ constexpr bool
>+ empty() const noexcept
>+ {
>+ return size() == 0;
>+ }
>+
>+ // observers: iterators
>+ constexpr iterator
>+ begin() const noexcept
>+ {
>+ return iterator(this->_M_ptr);
>+ }
>+
>+ constexpr const_iterator
>+ cbegin() const noexcept
>+ {
>+ return const_iterator(this->_M_ptr);
>+ }
>+
>+ constexpr iterator
>+ end() const noexcept
>+ {
>+ return iterator(this->_M_ptr + this->size());
>+ }
>+
>+ constexpr const_iterator
>+ cend() const noexcept
>+ {
>+ return const_iterator(this->_M_ptr + this->size());
>+ }
>+
>+ constexpr reverse_iterator
>+ rbegin() const noexcept
>+ {
>+ return reverse_iterator(this->begin());
>+ }
The rbeing() function needs to return an iterator that starts from the
end, i.e. return reverse_iterator(end());
>+ constexpr const_reverse_iterator
>+ crbegin() const noexcept
>+ {
>+ return const_reverse_iterator(this->cbegin());
Same here.
>+ }
>+
>+ constexpr reverse_iterator
>+ rend() const noexcept
>+ {
>+ return reverse_iterator(this->end());
And this should be return reverse_iterator(begin());
>+ }
>+
>+ constexpr const_reverse_iterator
>+ crend() const noexcept
>+ {
>+ return const_reverse_iterator(this->cend());
Same here.
>+ }
>+
>+ // observers: subranges
>+ template<size_t _Count>
>+ constexpr auto
It doesn't seem beneficial to use an 'auto' return type here.
>+ first() const
>+ {
>+ using __span_t = ::std::span<element_type, _Count>;
>+ return __span_t(this->data(), _Count);
>+ }
The function would be shorter without 'auto' return, and users can see
what it returns without inspecting the function body:
template<size_t _Count>
constexpr span<element_type, _Count>
first() const
{ return { data(), _Count }; }
Although we should put an assertion in there:
template<size_t _Count>
constexpr span<element_type, _Count>
first() const
{
__glibcxx_assert(_Count < size());
return { data(), _Count };
}
>+ constexpr auto
>+ first(index_type __count) const
>+ {
>+ using __span_t = ::std::span<element_type, dynamic_extent>;
>+ return __span_t(this->data(), __count);
Same here.
>+ }
>+
>+ template<size_t _Count>
>+ constexpr ::std::span<element_type, _Count>
>+ last() const
>+ {
>+ static_assert(_Count == dynamic_extent ||
>+ _Extent == dynamic_extent || _Count <= _Extent,
>+ "assertion failed: Count or Extent are dynamic, "
>+ "or the Count is less than the static extent");
Please also add a runtime assertion to check _Count <= size().
>+ using __span_t = ::std::span<element_type, _Count>;
>+ return __span_t(this->data() + (this->size() - _Count), _Count);
The return type is not needed here, it can be simply:
return {this->data() + (this->size() - _Count), _Count};
>+ }
>+
>+ constexpr auto
>+ last(index_type __count) const
>+ {
>+ using __span_t = ::std::span<element_type, dynamic_extent>;
>+ return __span_t(this->data() + (this->size() - __count), __count);
>+ }
>+
>+ template<size_t _Offset,
>+ size_t _Count = dynamic_extent>
>+ constexpr auto
>+ subspan() const
>+ {
>+ static_assert(_Count == dynamic_extent ||
>+ _Extent == dynamic_extent ||
>+ (_Offset + _Count) <= _Extent,
>+ "assertion failed: Count or Extent are dynamic, "
>+ "or the Count + Offset is less than the static extent");
>+ constexpr size_t __span_extent =
>+ (_Count != dynamic_extent
>+ ? _Count
>+ : (_Extent != dynamic_extent ? _Extent - _Offset
>+ : dynamic_extent));
>+ using __span_t = ::std::span<element_type, __span_extent>;
>+ return __span_t(this->data() + _Offset,
>+ (_Count == dynamic_extent ? this->size() - _Offset : _Count));
>+ }
>+
>+ constexpr auto
>+ subspan(
>+ index_type __offset, index_type __count = dynamic_extent) const
>+ {
>+ using __span_t = ::std::span<element_type, dynamic_extent>;
>+ return __span_t(this->data() + __offset,
>+ __count == dynamic_extent ? this->size() - __offset : __count);
>+ }
>+
>+ // observers: range helpers
>+ friend constexpr iterator
>+ begin(span __sp) noexcept
>+ {
>+ return __sp.begin();
>+ }
>+
>+ friend constexpr iterator
>+ end(span __sp) noexcept
>+ {
>+ return __sp.end();
>+ }
>+
>+ private:
>+ pointer _M_ptr;
>+ };
>+
>+ template<typename _Type, size_t _Extent>
>+ auto as_bytes(::std::span<_Type, _Extent> __sp) noexcept
>+ {
>+ constexpr size_t __byte_extent =
>+ (_Extent == ::std::dynamic_extent
>+ ? _Extent
>+ : (_Extent *
>+ sizeof(typename ::std::span<_Type, _Extent>::element_type)));
>+ using __byte_span_t = ::std::span<const byte, __byte_extent>;
>+ return __byte_span_t(
>+ reinterpret_cast<const byte*>(__sp.data()), __sp.size_bytes());
>+ }
Wouldn't this be easier to read (and easier for the compiler to
evaluate) if written how it's specified in the standard?
template<typename _Type, size_t _Extent>
span<const byte, _Extent == dynamic_extent
? dynamic_extent : _Extent * sizeof(_Type)>
as_bytes(span<_Type, _Extent> __sp) noexcept
{
return {reinterpret_cast<const byte*>(__sp.data()), __sp.size_bytes()};
}
>+ template<typename _Type, size_t _Extent>
>+ auto as_writable_bytes(::std::span<_Type, _Extent> __sp) noexcept
>+ {
>+ constexpr size_t __byte_extent =
>+ (_Extent == dynamic_extent
>+ ? _Extent
>+ : (_Extent *
>+ sizeof(typename ::std::span<_Type, _Extent>::element_type)));
>+ using __byte_span_t = ::std::span<byte, __byte_extent>;
>+ return __byte_span_t(
>+ reinterpret_cast<byte*>(__sp.data()), __sp.size_bytes());
>+ }
template<typename _Type, size_t _Extent>
span<byte, _Extent == dynamic_extent
? dynamic_extent : _Extent * sizeof(_Type)>
as_writable_bytes(span<_Type, _Extent> __sp) noexcept
{
return {reinterpret_cast<byte*>(__sp.data()), __sp.size_bytes()};
}
>+
>+ // tuple helpers
>+ template<size_t _Index, typename _Type, size_t _Extent,
>+ enable_if_t<(_Extent > 0) && (_Index < _Extent)>* = nullptr>
The standard says this check should be "Mandates:" not "Constraints:"
so it should be checked as a static_assert, not using SFINAE. Also,
isn't _Extent > 0 wrong? It must be greater than zero if _Index <
_Extent, and it should be checking _Extent != dynamic_extent according
to the curent draft.
>+ constexpr typename ::std::span<_Type, _Extent>::reference get(
Please start a new line for "get(" instead of having it wandering off
somewhere on the right. Shouldn't the return type be simply _Type& ?
So:
template<size_t _Index, typename _Type, size_t _Extent>
constexpr _Type&
get(span<_Type, _Extent> __sp) noexcept
{
static_assert(_Extent != dynamic_extent && _Index < _Extent);
return __sp[_Index];
}
>+ ::std::span<_Type, _Extent> __sp) noexcept
>+ {
>+ return __sp[_Index];
>+ }
>+
>+ template<typename _Type, size_t _Extent>
>+ class tuple_size<::std::span<_Type, _Extent>>
Please use 'struct' not 'class' for all the tuple_size and
tuple_element specializations. I've created
https://github.com/cplusplus/draft/pull/3211 to make that consistent
in the draft.
>+ : public integral_constant<size_t, _Extent>
>+ {
>+ };
>+
>+ template<typename _Type>
>+ class tuple_size<::std::span<_Type, dynamic_extent>>;
If you put static_assert(_Extent != dynamic_extent) in the partial
specialization above then you wouldn't need this incomplete type, and
it might give a more user-friendly diagnostic.
>+ template<size_t _Index, typename _Type, size_t _Extent>
>+ struct tuple_element<_Index, ::std::span<_Type, _Extent>>
>+ {
>+ static_assert(_Index < _Extent, "assertion failed: Index is less than Extent");
The compiler will already print "assertion failed" for a failed static
assertion, so this message would mean it's printed twice.
I'd leave the message, and just do static_assert(_Index < _Extent);
I think you also want static_assert(_Extent != dynamic_extent);
>+ using type = typename ::std::span<_Type, _Extent>::element_type;
>+ };
>+
>+ // deduction guides
Please put the deduction guides immediately after the class definition
(that's where we put them for all other types).
More information about the Gcc-patches
mailing list