Re: [ PATCH ] C++20 <span>

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

Jonathan Wakely-3
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.

>- P1394 might be slated to end up in C++20 per National Body Comments.
>Therefore, an early implementation is left in an
>implementation-defined _GLIBCXX_P1394 define.
>
>2019-08-30  JeanHeyd "ThePhD" Meneide  <[hidden email]>
>
>        * include/std/span: Implement the entirety of span.
>        * 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

>+++ b/libstdc++-v3/include/std/span
>@@ -0,0 +1,549 @@
>+// Components for manipulating non-owning sequences of objects -*- C++ -*-
>+
>+// Copyright (C) 2019-2019 Free Software Foundation, Inc.

Just 2019 please, not 2019-2019.

>+// WARNING: they forgot this feature test macro
>+// get on someone's back about it in Belfast!!!

Please use FIXME: instead of WARNING: (for consistency with the rest
of the sources, so people grepping for FIXME: can find this).

The new feature test macro should be in <version> too.

There's no need to qualify ::std::true_type and ::std::size_t when
within namespace std already. There's no ADL for type names, and
normal unqualified lookup will find the right ones (and is easier to
read).

>+      static_assert(
>+        _Count == ::std::dynamic_extent || _Extent == ::std::dynamic_extent || _Count <= _Extent,
>+        "bad span length");

There are a number of lines that are too long, they need to be broken
before 80 columns.

Our static_assert messages should be stated as the positive condition
that is being asserted. So the diagnostic reads like
"assertion failed: thing being asserted"

So "bad span length" makes it look like we asserted the length is bad,
but actually it was good. I prefer to write something saying "X must
be true", e.g. "count must be equal to dynamic_extent, or less than
the span's extent".

Do we need to check _Extent == ::std::dynamic_extent here, give nthat
if it's true then _Count <= _Extent will be true as well?

>+  std::vector<std::int_least32_t> value{ 0 };

Your new testcase uses std::vector without including <vector>.


Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

JeanHeyd Meneide
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!

On Fri, Aug 30, 2019 at 5:11 PM JeanHeyd Meneide
<[hidden email]> wrote:

>
> On Fri, Aug 30, 2019 at 3:41 PM Jonathan Wakely <[hidden email]> 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  <[hidden email]>
>
>         * 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

ThePhD.span.patch.txt (55K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

JeanHeyd Meneide
     Thank you for the thorough review!

On Tue, Sep 3, 2019 at 9:31 AM Jonathan Wakely <[hidden email]> wrote:
> It looks like__adl_begin and __adl_end should be guarded by #ifdef
> _GLIBCXX_P1394 because they're not needed otherwise.

     I'm absolutely going to need it for the bit data structures (and
in tuning up other parts of the library). It will also be important
for things that need to behave nicely when working with things needing
[range.access] internally in libstdc++,
while we wait for the cmcstl2 implementation. I did gate
__adl_to_address behind p1394, though, because that is definitely
specific to that paper (and its friend, p1474).

     Let me know if I missed anything!

     Tested on x64_86 Linux with 23_containers/* tests.

ThePhD.span.patch.txt (67K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

Jonathan Wakely-3
On 04/09/19 18:47 -0400, JeanHeyd Meneide wrote:

>     Thank you for the thorough review!
>
>On Tue, Sep 3, 2019 at 9:31 AM Jonathan Wakely <[hidden email]> wrote:
>> It looks like__adl_begin and __adl_end should be guarded by #ifdef
>> _GLIBCXX_P1394 because they're not needed otherwise.
>
>     I'm absolutely going to need it for the bit data structures (and
>in tuning up other parts of the library). It will also be important
>for things that need to behave nicely when working with things needing
>[range.access] internally in libstdc++,
>while we wait for the cmcstl2 implementation. I did gate
>__adl_to_address behind p1394, though, because that is definitely
>specific to that paper (and its friend, p1474).
OK, then it's fine as is, thanks.

>+/** @file span
>+ *  This is a Standard C++ Library header.
>+ */
>+
>+//
>+// P0122 span library
>+// Contributed by ThePhD
>+//
>+
>+#ifndef _GLIBCXX_SPAN
>+#define _GLIBCXX_SPAN 1
>+
>+#pragma GCC system_header
>+
>+#if __cplusplus > 201703L
>+
>+#include <cstddef>
This header isn't needed. <bits/c++config.h> defines std::size_t and
std::ptrdiff_t, and every libstdc++ header should include
<bits/c++config.h> already. Not including <cstddef> is a minor
optimisation for compile time, because we don't need to open <cstddef>
and <stddef.h>.

>+    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>
>+      class __extent_storage
>+      {
>+      public:
>+
>+        constexpr
>+        __extent_storage() noexcept = default;
Something else I forgot to mention (which I'll add to the coding
conventions documentation) is that we use TABs for indentation at the
start of lines. Tabstop should be set to 8, so anything indented by 8
chars or more should use TABs. Anything indented by fewer than 8 chars
just uses spaces.

>+
>+        constexpr
>+        __extent_storage(size_t) noexcept
>+        {}
>+
>+        static constexpr size_t
>+        _M_extent() noexcept
>+        { return _Extent; }
>+      };
>+
>+    template<>
>+      class __extent_storage<static_cast<size_t>(-1)>
>+      {
>+      public:
>+
>+        constexpr
>+        __extent_storage() noexcept : _M_extent_value(0){};
>+
>+        constexpr
>+        __extent_storage(size_t __extent) noexcept
>+        : _M_extent_value(__extent)
>+        {}
>+
>+        constexpr size_t
>+        _M_extent() const noexcept
>+        { return this->_M_extent_value; }
>+
>+      private:
>+        size_t _M_extent_value;
>+      };
>+
>+  } // namespace __detail
>+
>+  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
>+      template <typename _Dummy = _Type, enable_if_t<is_same_v<_Dummy, _Type> && (_Extent == dynamic_extent || _Extent == 0)>* = nullptr>
This line is too long, it needs to be split before 80 columns.

>+        constexpr span() noexcept : __base_t(0), _M_ptr(nullptr)
>+        {}
>+
>+      constexpr
>+      span(const span&) noexcept = default;
>+
>+      template<size_t _ArrayExtent,
>+        enable_if_t<
>+          (_Extent == dynamic_extent || _ArrayExtent == _Extent) &&

N.B. line breaks should come before logical operators like && e.g.

      foo
      && bar

Not:

      foo &&
      bar

The rationale for this is to make it easy to see the operator. You can
look at the start of  the line (at a predictable, easily findable
position) instead of having to look at the right hand side to see if
it's a && or || operator.


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

Here's a good example where the && operators are all at different
columns, but if you put line breaks before the operator it looks like:

          (_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>>
          && ...

This is easier to see that it's a series of AND conditions with a
quick glance.

As discussed on IRC the new XFAIL tests need some adjustment. The
attached patch changes the whitespace as discussed above, and fixes
the tests.

Our convention for XFAIL tests is to call them foo_neg.cc (not
fail.foo.cc like libc++ uses).

The attached patch is what I've tested and committed to trunk - thanks
very much!

I'll make some additional fixes (e.g. LWG 3103) and update the docs
later today.


patch.txt (49K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

Jonathan Wakely-3
On 05/09/19 12:27 +0100, Jonathan Wakely wrote:

>On 04/09/19 18:47 -0400, JeanHeyd Meneide wrote:
>>    Thank you for the thorough review!
>>
>>On Tue, Sep 3, 2019 at 9:31 AM Jonathan Wakely <[hidden email]> wrote:
>>>It looks like__adl_begin and __adl_end should be guarded by #ifdef
>>>_GLIBCXX_P1394 because they're not needed otherwise.
>>
>>    I'm absolutely going to need it for the bit data structures (and
>>in tuning up other parts of the library). It will also be important
>>for things that need to behave nicely when working with things needing
>>[range.access] internally in libstdc++,
>>while we wait for the cmcstl2 implementation. I did gate
>>__adl_to_address behind p1394, though, because that is definitely
>>specific to that paper (and its friend, p1474).
>
>OK, then it's fine as is, thanks.
>
>>+/** @file span
>>+ *  This is a Standard C++ Library header.
>>+ */
>>+
>>+//
>>+// P0122 span library
>>+// Contributed by ThePhD
>>+//
>>+
>>+#ifndef _GLIBCXX_SPAN
>>+#define _GLIBCXX_SPAN 1
>>+
>>+#pragma GCC system_header
>>+
>>+#if __cplusplus > 201703L
>>+
>>+#include <cstddef>
>
>This header isn't needed. <bits/c++config.h> defines std::size_t and
>std::ptrdiff_t, and every libstdc++ header should include
><bits/c++config.h> already. Not including <cstddef> is a minor
>optimisation for compile time, because we don't need to open <cstddef>
>and <stddef.h>.
>
>>+    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>
>>+      class __extent_storage
>>+      {
>>+      public:
>>+
>>+        constexpr
>>+        __extent_storage() noexcept = default;
>
>Something else I forgot to mention (which I'll add to the coding
>conventions documentation) is that we use TABs for indentation at the
>start of lines. Tabstop should be set to 8, so anything indented by 8
>chars or more should use TABs. Anything indented by fewer than 8 chars
>just uses spaces.
>
>>+
>>+        constexpr
>>+        __extent_storage(size_t) noexcept
>>+        {}
>>+
>>+        static constexpr size_t
>>+        _M_extent() noexcept
>>+        { return _Extent; }
>>+      };
>>+
>>+    template<>
>>+      class __extent_storage<static_cast<size_t>(-1)>
>>+      {
>>+      public:
>>+
>>+        constexpr
>>+        __extent_storage() noexcept : _M_extent_value(0){};
>>+
>>+        constexpr
>>+        __extent_storage(size_t __extent) noexcept
>>+        : _M_extent_value(__extent)
>>+        {}
>>+
>>+        constexpr size_t
>>+        _M_extent() const noexcept
>>+        { return this->_M_extent_value; }
>>+
>>+      private:
>>+        size_t _M_extent_value;
>>+      };
>>+
>>+  } // namespace __detail
>>+
>>+  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
>>+      template <typename _Dummy = _Type, enable_if_t<is_same_v<_Dummy, _Type> && (_Extent == dynamic_extent || _Extent == 0)>* = nullptr>
>
>This line is too long, it needs to be split before 80 columns.
>
>>+        constexpr span() noexcept : __base_t(0), _M_ptr(nullptr)
>>+        {}
>>+
>>+      constexpr
>>+      span(const span&) noexcept = default;
>>+
>>+      template<size_t _ArrayExtent,
>>+        enable_if_t<
>>+          (_Extent == dynamic_extent || _ArrayExtent == _Extent) &&
>
>N.B. line breaks should come before logical operators like && e.g.
>
>     foo
>     && bar
>
>Not:
>
>     foo &&
>     bar
>
>The rationale for this is to make it easy to see the operator. You can
>look at the start of  the line (at a predictable, easily findable
>position) instead of having to look at the right hand side to see if
>it's a && or || operator.
>
>
>>+      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>> &&
>
>Here's a good example where the && operators are all at different
>columns, but if you put line breaks before the operator it looks like:
>
>         (_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>>
>         && ...
>
>This is easier to see that it's a series of AND conditions with a
>quick glance.
>
>As discussed on IRC the new XFAIL tests need some adjustment. The
>attached patch changes the whitespace as discussed above, and fixes
>the tests.
>
>Our convention for XFAIL tests is to call them foo_neg.cc (not
>fail.foo.cc like libc++ uses).
>
>The attached patch is what I've tested and committed to trunk - thanks
>very much!
>
>I'll make some additional fixes (e.g. LWG 3103) and update the docs
>later today.
Here's the follow-up patch.

Tested x86_64-linux, committed to trunk.



patch.txt (43K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [ PATCH ] C++20 <span>

Jonathan Wakely-3
And some further simplifications and improvements to the constructor
constraints for std::span.

    This patch simplifies the constraints on the constructors from arrays by
    removing the redundant checks that element_type and value_type are
    convertible to element_type. The incorrect uses of __adl_data in those
    constructors are removed as well (they should use std::data not
    std::ranges::data, and the former doesn't use ADL).

    The range/container constructors are now constrained to exclude all
    specializations of std::span, not just the current instantiation. The
    range constructor now also checks s subset of the contiguous_range
    requirements.

    All relevant constructor constraints now use the _Require helper in
    order to short circuit and avoid unnecessary instantiations after the
    first failed constraint.

    A new constructor supports initialization from different specializations
    of std::span<OtherType, OtherExtent>, as specified in the C++20 draft.


Tested x86_64-linux, committed to trunk.



patch.txt (15K) Download Attachment