[PATCH] [RFC] span-v2: using concepts, constraints and ranges:: instead of enable_if
Jonathan Wakely
jwakely@redhat.com
Thu Nov 21 21:12:00 GMT 2019
On 21/11/19 13:33 -0500, JeanHeyd Meneide wrote:
>This is an attempt to use concepts and constraints rather than
>ranges::. It builds and run the tests I ran on x86_64 Linux decently
>(albeit I'm still new at doing this). I wanted to get a feel for
>whether or not this had been done right:
Thanks for doing this.
>Thoughts?
Needs moar changelog.
>diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
>index 6300de9e96d..7f7035ca64e 100644
>--- a/libstdc++-v3/include/Makefile.am
>+++ b/libstdc++-v3/include/Makefile.am
>@@ -156,6 +156,7 @@ bits_headers = \
> ${bits_srcdir}/random.tcc \
> ${bits_srcdir}/range_access.h \
> ${bits_srcdir}/range_cmp.h \
>+ ${bits_srcdir}/range_concepts.h \
You've added this here, but not actually added a new header in the
patch.
>index 3843ba5d57f..c4a4e92fed0 100644
>--- a/libstdc++-v3/include/bits/iterator_concepts.h
>+++ b/libstdc++-v3/include/bits/iterator_concepts.h
>@@ -71,6 +71,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> { *__t } -> __can_reference;
> };
>
>+ template<typename _LeftType, typename _RightType>
>+ concept __is_compatible_pointer = is_convertible<_RightType(*)[], _LeftType(*)[]>::value;
This could use is_convertible_v.
"left type" and "right type" don't
mean much to me, I'd prefer to keep it conventional and _Tp/_Up or
something meaninful like _From/_To.
I'd prefer not to have this at namespace-scope at all, at least not
unless we gie it a more descriptive name than __is_compatible_pointer.
How are they compatible? What is the context?
Something like array-compatible would be more descriptive, since
that's the condition it's testing, and the context it's used in.
std::unique_ptr<T[]> has similar checks so could reuse this ... except
we can't use concepts in std::unique_ptr. For that reason, I think an
alias template would be better, so it can be used in span and
unique_ptr<T[]> e.g.
template<typename _Tp, typename _Up>
using __array_compatible = is_convertible<_Tp(*)[], _Up(*)[]>;
That could be used in unique_ptr and can be used in span's constraints
too.
>+
> // FIXME: needed due to PR c++/67704
> template<__detail::__dereferenceable _Tp>
> struct __iter_ref
>diff --git a/libstdc++-v3/include/bits/range_access.h b/libstdc++-v3/include/bits/range_access.h
>index de074460c16..66697ffc354 100644
>--- a/libstdc++-v3/include/bits/range_access.h
>+++ b/libstdc++-v3/include/bits/range_access.h
>@@ -34,6 +34,7 @@
>
> #if __cplusplus >= 201103L
> #include <initializer_list>
>+#include <concepts>
This is redundant, it's included by the next header anyway ...
> #include <bits/iterator_concepts.h>
>
> namespace std _GLIBCXX_VISIBILITY(default)
>@@ -336,19 +337,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> ssize(const _Tp (&)[_Num]) noexcept
> { return _Num; }
>
>- // "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); }
>-
>- template <typename _Container>
>- constexpr auto
>- __adl_data(_Container& __cont) noexcept(noexcept(data(__cont)))
>- { return data(__cont); }
>-
> #ifdef __cpp_lib_concepts
> namespace ranges
> {
>@@ -869,10 +857,68 @@ namespace ranges
> template<typename _Tp>
> concept range = __detail::__range_impl<_Tp&>;
>
>+ template<range _Range>
>+ using sentinel_t = decltype(ranges::end(std::declval<_Range&>()));
>+
>+ template<range _Range>
>+ using iterator_t = decltype(ranges::begin(std::declval<_Range&>()));
>+
>+ template<range _Range>
>+ using range_value_t = iter_value_t<iterator_t<_Range>>;
>+
>+ template<range _Range>
>+ using range_reference_t = iter_reference_t<iterator_t<_Range>>;
>+
>+ template<range _Range>
>+ using range_rvalue_reference_t
>+ = iter_rvalue_reference_t<iterator_t<_Range>>;
>+
>+ template<range _Range>
>+ using range_difference_t = iter_difference_t<iterator_t<_Range>>;
>+
>+ namespace __detail
>+ {
>+ template<typename _Tp>
>+ concept __forwarding_range = range<_Tp> && __range_impl<_Tp>;
>+ } // namespace __detail
>+
> /// [range.sized] The sized_range concept.
> template<typename _Tp>
> concept sized_range = range<_Tp>
>- && requires(_Tp& __t) { ranges::size(__t); };
>+ && requires(_Tp& __t) { ranges::size(__t); };template<typename>
>+ inline constexpr bool disable_sized_range = false;
>+
>+ // [range.refinements]
>+ template<typename _Range, typename _Tp>
>+ concept output_range
>+ = range<_Range> && output_iterator<iterator_t<_Range>, _Tp>;
>+
>+ template<typename _Tp>
>+ concept input_range = range<_Tp> && input_iterator<iterator_t<_Tp>>;
>+
>+ template<typename _Tp>
>+ concept forward_range
>+ = input_range<_Tp> && forward_iterator<iterator_t<_Tp>>;
>+
>+ template<typename _Tp>
>+ concept bidirectional_range
>+ = forward_range<_Tp> && bidirectional_iterator<iterator_t<_Tp>>;
>+
>+ template<typename _Tp>
>+ concept random_access_range
>+ = bidirectional_range<_Tp> && random_access_iterator<iterator_t<_Tp>>;
>+
>+ template<typename _Tp>
>+ concept contiguous_range
>+ = random_access_range<_Tp> && contiguous_iterator<iterator_t<_Tp>>
>+ && requires(_Tp& __t)
>+ {
>+ { ranges::data(__t) } -> same_as<add_pointer_t<range_reference_t<_Tp>>>;
>+ };
>+
>+ template<typename _Tp>
>+ concept common_range
>+ = range<_Tp> && same_as<iterator_t<_Tp>, sentinel_t<_Tp>>;
>
> // [range.iter.ops] range iterator operations
>
>@@ -1008,12 +1054,6 @@ namespace ranges
> }
> }
>
>- template<range _Range>
>- using iterator_t = decltype(ranges::begin(std::declval<_Range&>()));
>-
>- template<range _Range>
>- using range_difference_t = iter_difference_t<iterator_t<_Range>>;
>-
> template<range _Range>
> constexpr range_difference_t<_Range>
> distance(_Range&& __r)
>diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
>index 333d110b67e..a8b27764419 100644
>--- a/libstdc++-v3/include/std/ranges
>+++ b/libstdc++-v3/include/std/ranges
>@@ -43,6 +43,7 @@
> #include <iterator>
> #include <limits>
> #include <optional>
>+#include <bits/range_concepts.h>
Here's that new header again, which isn't in your patch.
>
> /**
> * @defgroup ranges Ranges
>@@ -55,66 +56,6 @@ namespace std _GLIBCXX_VISIBILITY(default)
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
> namespace ranges
> {
>- // [range.range] The range concept.
>- // Defined in <bits/range_iterator.h>
>- // template<typename> concept range;
Please leave this comment (although I have just realised it gets the
name wrong ... oops).
>diff --git a/libstdc++-v3/include/std/span b/libstdc++-v3/include/std/span
>index fcec22a6c57..09864dc8e13 100644
>--- a/libstdc++-v3/include/std/span
>+++ b/libstdc++-v3/include/std/span
>@@ -42,6 +42,7 @@
> #include <tuple>
> #include <utility>
> #include <array>
>+#include <concepts>
Redundant.
> #include <bits/stl_iterator.h>
> #include <bits/range_access.h>
>
>@@ -105,6 +106,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> size_t _M_extent_value;
> };
>
>+ // _GLIBCXX_RESOLVE_LIB_DEFECTS
>+ // 3255. span's array constructor is too strict
>+ template<typename _Type, size_t _Extent, typename _Tp, size_t _ArrayExtent>
>+ concept __is_compatible_array
>+ = (_Extent == dynamic_extent || _ArrayExtent == _Extent)
>+ && __is_compatible_pointer<_Type, _Tp>;
>+
>+ template<typename _Type, typename _Iter>
>+ concept __is_compatible_iterator
>+ = contiguous_iterator<_Iter>
>+ && is_lvalue_reference_v<iter_reference_t<_Iter>>
>+ && same_as<iter_value_t<_Iter>, remove_cvref_t<iter_reference_t<_Iter>>>
>+ && __is_compatible_pointer<_Type, remove_reference_t<iter_reference_t<_Iter>>>;
>+
>+ template<typename _Type, typename _Range>
>+ concept __is_compatible_range
>+ = __is_compatible_iterator<_Type, ranges::iterator_t<_Range>>;
> } // namespace __detail
>
> template<typename _Type, size_t _Extent = dynamic_extent>
>@@ -122,21 +140,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return dynamic_extent;
> }
>
>- template<typename _Tp>
>- using __is_compatible = is_convertible<_Tp(*)[], _Type(*)[]>;
>-
>- // _GLIBCXX_RESOLVE_LIB_DEFECTS
>- // 3255. span's array constructor is too strict
>- template<typename _Tp, size_t _ArrayExtent,
>- typename = enable_if_t<_Extent == dynamic_extent
>- || _ArrayExtent == _Extent>>
>- using __is_compatible_array = __is_compatible<_Tp>;
The advantage of defining these here, not at namespace-scope, is that
you don't need to repeat the class template's parameters here.
>-
> public:
> // member types
> using value_type = remove_cv_t<_Type>;
> using element_type = _Type;
>- using index_type = size_t;
>+ using size_type = size_t;
> using reference = element_type&;
> using const_reference = const element_type&;
> using pointer = _Type*;
>@@ -156,160 +164,74 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> // constructors
>
>- template<bool _DefaultConstructible = (_Extent + 1u) <= 1u,
>- enable_if_t<_DefaultConstructible>* = nullptr>
>- constexpr
>- span() noexcept : _M_extent(0), _M_ptr(nullptr)
>- { }
>+ constexpr
>+ span() noexcept
>+ requires ((_Extent + 1u) <= 1u)
>+ : _M_extent(0), _M_ptr(nullptr)
>+ { }
>
> constexpr
> span(const span&) noexcept = default;
>
>- template<typename _Tp, size_t _ArrayExtent,
>- typename = _Require<__is_compatible_array<_Tp, _ArrayExtent>>>
>+ template<typename _Tp, size_t _ArrayExtent>
>+ requires __detail::__is_compatible_array<_Type, _Extent, _Tp, _ArrayExtent>
> constexpr
> span(_Tp (&__arr)[_ArrayExtent]) noexcept
> : span(static_cast<pointer>(__arr), _ArrayExtent)
> { }
>
>- template<typename _Tp, size_t _ArrayExtent,
>- typename = _Require<__is_compatible_array<_Tp, _ArrayExtent>>>
>+ template<typename _Tp, size_t _ArrayExtent>
>+ requires __detail::__is_compatible_array<_Type, _Extent, _Tp, _ArrayExtent>
You only need four template arguments here because you defined it at
namespace-scope as a concept. What was wrong with the alias template?
You could have left the alias template and used it like:
requires __is_compatible_array<_Tp, _ArrayExtent>::value
> constexpr
> span(array<_Tp, _ArrayExtent>& __arr) noexcept
> : span(static_cast<pointer>(__arr.data()), _ArrayExtent)
> { }
>
>- template<typename _Tp, size_t _ArrayExtent,
>- typename = _Require<__is_compatible_array<_Tp, _ArrayExtent>>>
>+ template<typename _Tp, size_t _ArrayExtent>
>+ requires __detail::__is_compatible_array<_Type, _Extent, const _Tp, _ArrayExtent>
> constexpr
> span(const array<_Tp, _ArrayExtent>& __arr) noexcept
> : span(static_cast<pointer>(__arr.data()), _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?
>-#ifdef _GLIBCXX_P1394
>- private:
>- // FIXME: use std::iter_reference_t
>- template<typename _Iterator>
>- using iter_reference_t = decltype(*std::declval<_Iterator&>());
>- // FIXME: use std::ranges::iterator_t
>- // N.B. constraint is needed to prevent a cycle when __adl_begin finds
>- // begin(span) which does overload resolution on span(Range&&).
>- template<typename _Rng,
>- typename _Rng2 = remove_cvref_t<_Rng>,
>- typename = enable_if_t<!__detail::__is_std_span<_Rng2>::value>>
>- using iterator_t = decltype(std::__adl_begin(std::declval<_Rng&>()));
>- // FIXME: use std::iter_value_t
>- template<typename _Iter>
>- using iter_value_t = typename iterator_traits<_Iter>::value_type;
>- // FIXME: use std::derived_from concept
>- template<typename _Derived, typename _Base>
>- using derived_from
>- = __and_<is_base_of<_Base, _Derived>,
>- is_convertible<const volatile _Derived*, const volatile _Base*>>;
>- // FIXME: require contiguous_iterator<_Iterator>
>- template<typename _Iter,
>- typename _Ref = iter_reference_t<_Iter>,
>- typename _Traits = iterator_traits<_Iter>,
>- typename _Tag = typename _Traits::iterator_category>
>- using __is_compatible_iterator
>- = __and_<derived_from<_Tag, random_access_iterator_tag>,
>- is_lvalue_reference<_Ref>,
>- is_same<iter_value_t<_Iter>, remove_cvref_t<_Ref>>,
>- __is_compatible<remove_reference_t<_Ref>>>;
>-
>- template<typename _Range>
>- using __is_compatible_range
>- = __is_compatible_iterator<iterator_t<_Range>>;
>-
> public:
>- template<typename _Range, typename = _Require<
>- bool_constant<_Extent == dynamic_extent>,
>- __not_<__detail::__is_std_span<remove_cvref_t<_Range>>>,
>- __not_<__detail::__is_std_array<remove_cvref_t<_Range>>>,
>- __not_<is_array<remove_reference_t<_Range>>>,
>- __is_compatible_range<_Range>>,
>- typename = decltype(std::__adl_data(std::declval<_Range&>()))>
>+ template<ranges::contiguous_range _Range>
>+ requires (_Extent == dynamic_extent)
>+ && (!__detail::__is_std_span<remove_cvref_t<_Range>>::value)
>+ && (!__detail::__is_std_array<remove_cvref_t<_Range>>::value)
>+ && (!is_array<remove_reference_t<_Range>>::value)
is_array_v<...> instead of is_array<...>::value
>+ && __detail::__is_compatible_range<_Type, _Range>
> constexpr
> span(_Range&& __range)
>- noexcept(noexcept(::std::__adl_data(__range))
>- && noexcept(::std::__adl_size(__range)))
>- : span(::std::__adl_data(__range), ::std::__adl_size(__range))
>+ noexcept(noexcept(ranges::data(__range))
>+ && noexcept(ranges::size(__range)))
>+ : span(ranges::data(__range), ranges::size(__range))
> { }
>
>- template<typename _ContiguousIterator, typename _Sentinel, typename
>- = _Require<__not_<is_convertible<_Sentinel, index_type>>,
>- __is_compatible_iterator<_ContiguousIterator>>>
>+ template<contiguous_iterator _ContiguousIterator,
>+ sized_sentinel_for<_ContiguousIterator> _Sentinel>
>+ requires __detail::__is_compatible_iterator<_Type, _ContiguousIterator>
>+ && (!is_convertible<_Sentinel, size_type>::value)
is_convertible_v
More information about the Libstdc++
mailing list