SFINAE span default constructor on Extent == 0

[ldionne]

Summary

The default constructor of a static span requires _Extent == 0 so SFINAE it out rather than using a static_assert

[miscco]

better message

[mclow.lists]

What is the benefit to the user to have this constructor SFINAE away?
With static_assert we can craft an error message.

[miscco]

Hm, this fixes a todo that was written by you..

No seriously I believe this is indeed a correctness issue.

First the standard marks this as a "Constraint". As far as my standardese goes this requires that this function should be SFINAEd out.

Also if we would not remove the default constructor span<T, 2> would fulfill the`default_constructible` concept although it does not.

So I believe there is no alternative to removing that constructor.

[mclow.lists]

In D71994#1802434, @miscco wrote:

Also if we would not remove the default constructor span<T, 2> would fulfill the`default_constructible` concept although it does not.

That is a compelling argument.

Since you pointed this out, we probably need a test showing that this is true for:

• dynamically sized spans
• statically sized spans of size 0
• but NOT for statically sized spans of size > 0

[miscco]

Library concepts are not yet implemented.

I had a look at it over the weekend. The problem is that requires expressions are not yet implemented so we cannot even add those concepts yet. I didnt find out how to pass -fconcepts to gcc-9 in LIT and was too lazy to build everything on windows with MSVC.

So I would keep them as a todo?

[miscco]

• [libcxx] span: Check default constructible

[miscco]

Added type_traits and concepts check for the different spans

[CaseyCarter]

libcxx/test/std/containers/views/span.cons/default.pass.cpp

17 ↗

(On Diff #236609)

Late comment: <span> isn't guaranteed to export __cpp_lib_concepts; this should include <version> before checking __cpp_lib_concepts.

[miscco]

• [libcxx] span: Check default constructible

[miscco]

libcxx/test/std/containers/views/span.cons/default.pass.cpp

17 ↗

(On Diff #236609)

I added it one tooth-brushing with my kids ago. That feels like an eternity but if it qualifies as late...

[miscco]

Added missing include of <version>

[CaseyCarter]

LGTM. (Resigning as reviewer rather than approving to avoid confusing the folks who are actually authorized to approve.)

[miscco]

Gentle Ping

[miscco]

rebased

[miscco]

[libcxx][span] Use requires clause for default constructor

[miscco]

gentle ping

[EricWF]

Does this really need concepts?

libcxx/include/span

212–213

We don't use concepts anywhere else in the library. Why are you not using SFINAE instead?

What happens when you include this file when requires isn't supported?

[miscco]

You are absolutely right.

While it is technically correct as both span and concepts are a C++20 feature I belive this change is not helpful. It is just soo much better :(

That said, we should switch to concepts once the library concepts and ranges are fully available as that is needed for conformance of the range based contructors.

How do I revert an update in phabricator?

[miscco]

Do not require concepts

[miscco]

Ping, I guess the clang format should not be applied?

[ldionne]

All concerns by other reviewers seem to have been addressed, feel free to move forward. Don't apply clang-format changes. Thanks!

[ldionne]

I'm committing this, but I'm removing concepts from the tests.

Please don't require features we don't implement like that, it's just pointless. You basically have dead code in your test as it stands.

[ldionne]

Get rid of concepts

libcxx/include/span

//  constants and types
     using iterator               =  __wrap_iter<pointer>;
     using const_iterator         =  __wrap_iter<const_pointer>;
     using reverse_iterator       = _VSTD::reverse_iterator<iterator>;
     using const_reverse_iterator = _VSTD::reverse_iterator<const_iterator>;
 
     static constexpr size_type extent = _Extent;
 
 // [span.cons], span constructors, copy, assignment, and destructor
-    _LIBCPP_INLINE_VISIBILITY constexpr span() noexcept : __data{nullptr}
-    { static_assert(_Extent == 0, "Can't default construct a statically sized span with size > 0"); }
+    template <size_t _Sz = _Extent, enable_if_t<_Sz == 0, nullptr_t> = nullptr>
+    _LIBCPP_INLINE_VISIBILITY constexpr span() noexcept : __data{nullptr} {}
 
     constexpr span           (const span&) noexcept = default;
     constexpr span& operator=(const span&) noexcept = default;
 
     _LIBCPP_INLINE_VISIBILITY constexpr span(pointer __ptr, size_type __count) : __data{__ptr}
         { (void)__count; _LIBCPP_ASSERT(_Extent == __count, "size mismatch in span's constructor (ptr, len)"); }
     _LIBCPP_INLINE_VISIBILITY constexpr span(pointer __f, pointer __l) : __data{__f}
         { (void)__l;     _LIBCPP_ASSERT(_Extent == distance(__f, __l), "size mismatch in span's constructor (ptr, ptr)"); }

libcxx/test/std/containers/views/span.cons/default.fail.cpp

 // -*- C++ -*-
 //===------------------------------ span ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===---------------------------------------------------------------------===//
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 
 // <span>
 
 // constexpr span() noexcept;
 //
 //  Remarks: This constructor shall not participate in overload resolution
-//          unless Extent <= 0 is true.
+//          unless Extent == 0 || Extent == dynamic_extent is true.
 
 
 #include <span>
 #include <cassert>
 #include <string>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
-    std::span<int, 2> s; // expected-error-re@span:* {{static_assert failed{{( due to requirement '.*')?}} "Can't default construct a statically sized span with size > 0"}}
-
-//  TODO: This is what I want:
-// eXpected-error {{no matching constructor for initialization of 'std::span<int, 2>'}}
+  std::span<int, 2> s; // expected-error {{no matching constructor for initialization of 'std::span<int, 2>'}}
 
   return 0;
 }