This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P1394r4 for span: range constructor
ClosedPublic

Authored by jloser on Sep 26 2021, 8:47 AM.

Details

Summary

Implement https://wg21.link/p1394 which allows span to be constructible
from any contiguous forwarding-range that has a compatible element type.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51443

Diff Detail

Event Timeline

jloser requested review of this revision.Sep 26 2021, 8:47 AM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2021, 8:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser updated this revision to Diff 375116.Sep 26 2021, 8:53 AM

Use SimpleRange from "simple_range.h" in range.fail.cpp

Quuxplusone requested changes to this revision.Sep 26 2021, 10:12 AM
Quuxplusone added inline comments.
libcxx/include/span
69–71

This ctor should also be conditionally explicit, according to https://eel.is/c++draft/span.cons
[p1394r4] is missing all these explicits for some reason, so eel.is (or whatever copy of the working draft) should be our guide.

170–171

If you're updating these, might as well make them all use e.g.
enable_if_t<!__is_std_span<remove_cvref_t<_Tp>>::value>,

174

If we're assuming Ranges+Concepts support (contiguous_range), then we can use concept instead of inline constexpr bool, and make things maybe very slightly easier on the compiler. However, I'm not sure that we can assume Concepts support; don't we need some #ifdef guards sprinkled through here? Ditto on the deduction guides that use ranges:: things.

Also, the name of this should be something like __is_span_compatible_range; remember this is directly in namespace _VSTD along with everything else in the library, so its name shouldn't be too generic.

416

ADL-proof: _VSTD::to_address, _VSTD::distance. Ditto lines 236, 248.
(The LWG clauses of the Standard have a bad, but at least consistent, pattern, of never writing std:: when it is "obvious." This makes it hard for them to indicate when ADL is actually intended, e.g. on swap; but any time ADL isn't clearly intended, you should always read their foo as std::foo.)

I also (always, but especially here) strongly recommend using () for direct-initialization, never {}. Parens are always correct; braces are sometimes equivalent to parens and sometimes wrong (either because they get hijacked by initializer_list ctors, or because they prohibit narrowing conversions when LWG didn't say to prohibit them).

440–441

Slight scope-creep: For consistency with elsewhere, please rewrite the SFINAE as

template <class _Range, enable_if_t<__is_range_compatible<_Range, element_type>>* = nullptr>

The benefit (in general) is that it's possible to have two overloaded function templates with template-heads

template <class _Range, enable_if_t<A>* = nullptr>
template <class _Range, enable_if_t<B>* = nullptr>

whereas it is not possible to have two overloaded function templates with template-heads

template <class _Range, class = enable_if_t<A>>
template <class _Range, class = enable_if_t<B>>

(because those would be one template with two declarations, with two incompatible defaults for the defaulted parameter).

569–570

I believe we can write this as

template<contiguous_iterator _It, class _EndOrSize>
    span(_It, _EndOrSize) -> span<remove_reference_t<iter_reference_t<_It>>>;

Note the name change to _EndOrSize, to match the working draft. (This also self-documents why it's not constrained as sentinel_for<_It>: it's because it's not necessarily an _End; it might be a _Size instead, e.g. an int.)

Ditto line 586: template<ranges::contiguous_range _Range>.

libcxx/test/std/containers/views/span.cons/iterator_iterator.fail.cpp
48–54 ↗(On Diff #375116)

I'd prefer to see these as simple compile-time tests:

static_assert( std::is_constructible_v<std::span<int>, int*, int*>);
static_assert(!std::is_constructible_v<std::span<int>, const int*, const int*>);
static_assert(!std::is_constructible_v<std::span<int>, volatile int*, volatile int*>);
static_assert( std::is_constructible_v<std::span<const int>, int*, int*>);
static_assert( std::is_constructible_v<std::span<const int>, const int*, const int*>);
static_assert(!std::is_constructible_v<std::span<const int>, volatile int*, volatile int*>);
static_assert( std::is_constructible_v<std::span<volatile int>, int*, int*>);
static_assert(!std::is_constructible_v<std::span<volatile int>, const int*, const int*>);
static_assert( std::is_constructible_v<std::span<volatile int>, volatile int*, volatile int*>);

etc. And then similarly for std::is_convertible_v to test the non-explicit ctors.

I think it would be nice (but not necessary, and maybe excessively redundant) to put in some realistic tests that use span as a parameter-only type in the appropriate way:

void f1(std::span<int>) {}
void f2(std::span<const int>) {}
void fs1(std::span<int, 3>) {}
void fs2(std::span<const int, 3>) {}
int main() {
    std::vector<int> v = {1,2,3};
    f1(v);
    f2(v);
    f2(std::vector<int>{1,2,3});
    f1({v.data(), v.size()});
    f2({v.data(), v.size()});
    fs1(std::span{v.data(), v.size()});  // this works, right?
    fs2(std::span{v.data(), v.size()});  // this works, right?
}

and so on, just to touch all the relevant real-world use cases.

libcxx/test/std/containers/views/span.cons/iterator_len.pass.cpp
81–83
T val[2] = {};
auto s1 = std::span<T>(val, 2);
auto s2 = std::span<T, 2>(val, 2);
assert(s1.data() == &val[0] && s1.size() == 2);
assert(s2.data() == &val[0] && s2.size() == 2);
return true;

The big things here are that you can use assert in constexpr functions, and that you don't mark variables constexpr inside constexpr functions. I assumed that the const in const T was not significant to what you were trying to test; otherwise you could bring it back.
Probably you can collapse testConstexprSpan and testRuntimeSpan into the same function, now.

libcxx/test/std/containers/views/span.cons/range.fail.cpp
48–49 ↗(On Diff #375116)

This is redundant with the primary template (thus DisabledBorrowedRange is redundant with IsARange). Did you mean to try with a type that enabled borrowed-range for itself? I'd suggest just using string_view for that.

59–61 ↗(On Diff #375116)

No need to drag in all three of these; one will suffice. The relevant property is "being a range, but not being a contiguous range"... actually, waitaminute... these containers also are not borrowed ranges! So of course lines 59-61 won't compile, for a whole host of reasons. I don't think there's any reason to have lines 59-61 at all, now.

However, you might test a "borrowed but non-contiguous" rvalue range, and a "lvalue but non-contiguous" non-borrowed range. Again I recommend doing this as a compile-time static_assert, something like:

static_assert( std::is_constructible_v<std::span<int>, std::vector<int>&>);  // non-borrowed lvalue
static_assert(!std::is_constructible_v<std::span<int>, std::vector<int>&&>);  // non-borrowed rvalue
static_assert(!std::is_constructible_v<std::span<int>, std::deque<int>&>);  // non-contiguous lvalue
static_assert(!std::is_constructible_v<std::span<int>, std::subrange<std::deque<int>::iterator, std::deque<int>::iterator>&&>);  // non-contiguous borrowed rvalue
libcxx/test/std/containers/views/span.cons/simple_range.h
4 ↗(On Diff #375116)

Once you convert range.fail.cpp to a bunch of static_asserts, you'll be using SimpleRange in only one file, and then it won't need its own header, so this comment will be moot.

If this class isn't scoped to a single test, then it should have a descriptive name like SimpleContiguousNonborrowedRange... which is just std::vector, isn't it? What's the point of making our own type here at all?

This revision now requires changes to proceed.Sep 26 2021, 10:12 AM
jloser updated this revision to Diff 375252.Sep 27 2021, 7:30 AM

Address some review feedback:

  • Add some #ifdef for ranges support in span
  • s/__is_range_compatible/__is_span_compatible_range
  • Simplify deduction guides to use concepts rather than enable_if_t
  • Consistent SFINAE using = nullptr
  • ADL proof to_address and distance
  • Fix exposition with conditional explicitness on range constructor for span
jloser marked 6 inline comments as done.Sep 27 2021, 7:31 AM

Thanks a lot for working on this!

I spoke with Joe on Discord and we agreed to make the change as-is, i.e. provide the constructors only when ranges are supported. That implies that compilers like AppleClang 12.0 (I think that's the only supported one) will NOT have the old span(Container&) constructors anymore, which is a breaking change. However, AppleClang 13.0 was released on September 20, which means that per our compiler support policy, we'd drop official support for 12.0. I'll get started on that but it does require upgrading Xcode on the CI nodes, and that's going to cause some problems for back-deployment testing. TL;DR: let's do as-if we didn't support 12.0 anymore and just mark the failing tests as XFAIL. I'll remove those XFAILs once we remove support for 12.0.

libcxx/include/span
173

This should be

#if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_RANGES)

That way, once we drop support for compilers that don't implement concepts, we'll just get rid of && !defined(_LIBCPP_HAS_NO_RANGES) entirely.

jloser updated this revision to Diff 375430.Sep 27 2021, 4:14 PM
jloser edited the summary of this revision. (Show Details)

Try to fix #ifdef around ranges + concepts support assumptions
Make __is_span_compatible_range a concept
Convert most tests in iterator_iterator.fail.cpp into compile-time tests in iterator_iterator.compile.pass.cpp.

jloser added inline comments.Sep 27 2021, 4:29 PM
libcxx/include/span
174

Renamed to __is_span_compatible_range.

Talked with Louis on Discord and we decided we can assume ranges + concepts support. This will fail on CI for AppleClang 12.0 which we're dropping support for soon since AppleClang 13.0 just came out on 9/20. As such, made __is_span_compatible_range a concept which allows me to use terse syntax below in those constructors rather than enable_if_t<...>.

jloser added inline comments.Sep 27 2021, 4:31 PM
libcxx/include/span
173

I changed it to that, but keep in mind p1394 is C++20 and not C++23. So, what do you think about

#if defined(__cpp_lib_concepts) && !defined(_LIBCPP_HAS_NO_RANGES)

instead? This will ensure concepts supports and >= C++20.

jloser updated this revision to Diff 375436.Sep 27 2021, 4:52 PM
  • Simplify test functions in iterator_len.pass.cpp and iterator_iterator.pass.cpp since we can use assert in constexpr functions.
  • Mark new tests as UNSUPPORTED: libcpp-has-no-incomplete-ranges
jloser added inline comments.Sep 27 2021, 4:53 PM
libcxx/test/std/containers/views/span.cons/iterator_len.pass.cpp
81–83

I like that much better -- thanks! Just applied your suggestion and collapsed the previous testConstexprSpan and testRuntimeSpan. Similarly in iterator_iterator.pass.cpp.

jloser marked an inline comment as done.Sep 27 2021, 4:53 PM
jloser updated this revision to Diff 375441.Sep 27 2021, 5:29 PM
jloser edited the summary of this revision. (Show Details)

Simplify range.pass.cpp.

  • TODO: convert check_cv to using std::is_constructible_v
jloser added inline comments.Sep 27 2021, 5:31 PM
libcxx/test/std/containers/views/span.cons/simple_range.h
4 ↗(On Diff #375116)

Its new use in range.pass.cpp will be that we can have our test() function be usable in a constexpr context since std::vector isn't fully constexpr yet.

libcxx/test/std/containers/views/span.cons/simple_range.h
4 ↗(On Diff #375116)

Makes sense, but for that, you might be able to get away with either int[10] or std::array<int,10> or libcxx/test/support/test_constexpr_container.h (which right now is used only in the back_inserter tests IIRC).

jloser updated this revision to Diff 375733.Sep 28 2021, 4:34 PM
jloser edited the summary of this revision. (Show Details)

Simplify tests from Arthur's feedback

jloser updated this revision to Diff 375735.Sep 28 2021, 4:38 PM

Remove dead code from range.compile.pass.cpp

jloser updated this revision to Diff 375736.Sep 28 2021, 4:41 PM

Remove reliance on SimpleRange from deduct.pass.cpp
Remove simple_range.h

jloser marked an inline comment as done.Sep 28 2021, 4:41 PM
jloser added inline comments.
libcxx/test/std/containers/views/span.cons/simple_range.h
4 ↗(On Diff #375116)

Ended up not needing it in favor of our favorite friend the C-style array. Removed simple_range.h, etc.

jloser marked an inline comment as done.Sep 28 2021, 4:47 PM
jloser added inline comments.
libcxx/test/std/containers/views/span.cons/iterator_iterator.fail.cpp
48–54 ↗(On Diff #375116)

I reworked several tests (itr/itr, itr/len, range) to mostly be compile-time tests that use std::is_constructible_v and std::is_convertible_v and I like that a lot better!

I did leave iterator_iterator.fail.cpp and iterator_len.fail.cpp which test the explicit constructors respectively. I don't see how we can use std::is_convertible_v since the constructor takes more than one parameter (two in each of these cases), but we only have one template type for the from type in std::is_convertible_v. Am I missing something?

As for the idea around adding tests that we can call a function taking a span parameter, I don't immediately see the value-add it has from a coverage perspective. I haven't added them now, but if you can convince me what coverage it adds, I'm happy to add them.

jloser updated this revision to Diff 375740.Sep 28 2021, 4:52 PM

Remove extra scope in iterator_iterator.fail.cpp

jloser updated this revision to Diff 375772.Sep 28 2021, 8:18 PM

Mark a few tests as UNSUPPORTED: libcpp-no-concepts
clang-format a few places

jloser updated this revision to Diff 375779.Sep 28 2021, 9:51 PM

Collapse .compile.pass.cpp into their respective .pass.cpp files
Try to fix #ifdefs to make CI happy

jloser updated this revision to Diff 375781.Sep 28 2021, 9:54 PM

Remove extra test in deduct.pass.cpp

jloser updated this revision to Diff 375881.Sep 29 2021, 7:26 AM

Prefer test() over assert(test()) in a few tests

@ldionne @Quuxplusone can you help me understand why a few static_asserts fail on the CI job (https://reviews.llvm.org/harbormaster/unit/view/1268018/)? They pass locally just fine for me. Everything else modulo formatting was passing.

@ldionne @Quuxplusone can you help me understand why a few static_asserts fail on the CI job (https://reviews.llvm.org/harbormaster/unit/view/1268018/)? They pass locally just fine for me. Everything else modulo formatting was passing.

Well, your new code in <span> is enabled only in language versions greater than 20, but your new test runs in versions greater than or equal to 20. Did you intend the test to also UNSUPPORTED: c++20? or did you intend the new code to be enabled when _LIBCPP_STD_VER > 17? (Is this a C++2b feature or a C++20 feature? I think it's C++20, but check me on that.)

libcxx/include/span
182

Nit: s/>> (/>>(/ for consistency with _ElementType(*)[].

213–238

Nit:

template <class _It,
          enable_if_t<contiguous_iterator<_It> &&
                      is_convertible_v<remove_reference_t<iter_reference_t<_It>>(*)[], element_type(*)[]>>* = nullptr>

(Cuddling > > (, and using enable_if_t<X>* = nullptr (note the *) instead of enable_if_t<X, nullptr_t> = nullptr.)

libcxx/include/span
443–449

Scope-creep: It would be nice to refactor this SFINAE, for consistency, as

template <class _OtherElementType, size_t _OtherExtent,
          enable_if_t<is_convertible_v<_OtherElementType(*)[], element_type(*)[]>>* = nullptr>
_LIBCPP_INLINE_VISIBILITY
constexpr span(const span<_OtherElementType, _OtherExtent>& __other) noexcept
        : __data(__other.data()), __size(__other.size()) {}

and so on throughout this file. But I could submit a separate (subsequent) PR for this myself. (Heck, I could even underscore-suffix the data members __data and __size!)

jloser updated this revision to Diff 375941.Sep 29 2021, 10:13 AM

Enable range constructor in C++20 mode, not > C++20 which should fix CI
Fix some clang-format mishaps with cuddling of >> vs > >.

@ldionne @Quuxplusone can you help me understand why a few static_asserts fail on the CI job (https://reviews.llvm.org/harbormaster/unit/view/1268018/)? They pass locally just fine for me. Everything else modulo formatting was passing.

Well, your new code in <span> is enabled only in language versions greater than 20, but your new test runs in versions greater than or equal to 20. Did you intend the test to also UNSUPPORTED: c++20? or did you intend the new code to be enabled when _LIBCPP_STD_VER > 17? (Is this a C++2b feature or a C++20 feature? I think it's C++20, but check me on that.)

Oops! It's intended to be a C++20 feature (that is, P1394 targets C++20 not C++23), and since std::span is C++20, we just need the #ifdef around ranges support. Just fixed that -- thanks!.

jloser added inline comments.Sep 29 2021, 10:16 AM
libcxx/include/span
443–449

I'd vote let's just do that consistently throughout this file in a follow-up PR if that's OK with you.

ldionne accepted this revision as: ldionne.Sep 29 2021, 12:50 PM

I'm relying on Arthur's in-depth review of the tests. Ship it once my comments have been resolved, everybody else is happy and CI is passing. Thanks a lot!

libcxx/include/span
173

I had made my comment thinking this was a C++23 feature, but what you have right now (just #if !defined(_LIBCPP_HAS_NO_RANGES)) seems like the correct thing to me.

217

Here and elsewhere.

libcxx/test/std/containers/views/span.cons/iterator_len.fail.cpp
1 ↗(On Diff #375941)

Here and throughout.

4 ↗(On Diff #375941)

Let's also rename this test to iterator_len.verify.cpp, since it's a verify-style test.

Note that we support verify-style failures in .fail.cpp tests for historical reasons only and we should move away from doing that, hence my request.

jloser updated this revision to Diff 376008.Sep 29 2021, 1:00 PM

Address Louis's feedback

jloser marked an inline comment as done.Sep 29 2021, 1:00 PM
jloser updated this revision to Diff 376079.Sep 29 2021, 5:27 PM

Fix debug iterators CI build due to typo in _LIBCPP_ASSERT call

jloser updated this revision to Diff 376256.Sep 30 2021, 9:50 AM

Sprinkle some #if !defined(_LIBCPP_HAS_NO_RANGES) to guard against not having iter_reference_t and friends

ping @Quuxplusone - what do you think of the tests now? I reworked a lot of them to be compile-time tests where possible.

Note: Please wait until CI is back online and passing before shipping this. It would suck to come back Monday and have a bunch of failures to investigate :(

jloser added a comment.Oct 1 2021, 2:18 PM

Note: Please wait until CI is back online and passing before shipping this. It would suck to come back Monday and have a bunch of failures to investigate :(

No problem, I'm in no rush to land this. We'll wait until CI is back and passing and Arthur thinks the tests are in good shape before we ship it.

Quuxplusone requested changes to this revision.Oct 1 2021, 4:47 PM

Mostly nits, but "this helper trait is now completely unused" is significant enough that I think we'd benefit from one more round.

libcxx/include/span
70

s/extent/Extent/

170–171

Actually, isn't this type-trait completely unused now? Let's remove lines 172–194!

If I'm wrong and it is used somewhere, then please add a test for

struct Widget { friend int size(Widget*); };
static_assert(!std::is_constructible_v<std::span<Widget>, Widget(&)[]>);

and remove the , nullptr_t on lines 179 and 181.

174

Belatedly altering my advice a little, sorry: If this is a concept, especially if it's being used with the terse syntax, then let's call it __span_compatible_range (with no is_ prefix).
Also, line 201 has one bogus extra space.

416
  • Parens-not-braces for initializations.
  • Actually _VSTD::distance(__first, __last) is definitely wrong; please add some tests where the iterator and sentinel types are different. This should be simply __last - __first.
libcxx/test/std/containers/views/span.cons/iterator_iterator.pass.cpp
16 ↗(On Diff #376256)

s/extent/Extent/g

33 ↗(On Diff #376256)

s/tability/tibility/
and likewise in at least one other file

libcxx/test/std/containers/views/span.cons/iterator_iterator.verify.cpp
21 ↗(On Diff #376256)

s/extent/Extent/
I'm beginning to suspect some automated process was responsible for lowercasing all of these Extents. :)

29 ↗(On Diff #376256)

createImplicitSpan<int, 1>(arr, arr+1)... although, why 1, if the array has 3 elements? Perhaps createImplicitSpan<int, 3>(arr, arr+3) would get the point across better.

Btw, you could turn this into a SFINAE test and get rid of the .verify.cpp. The SFINAE test would look basically like https://godbolt.org/z/xf6sn8asf

template<class T, size_t Extent> bool f(int, std::span<T, Extent>) { return true; }
template<class, size_t> bool f(long, std::pair<int*, int*>) { return false; }

    int a[3] = {1,2,3};
    assert((f<int, std::dynamic_extent>(42, {a, a+3})));
    assert((!f<int, 3>(42, {a, a+3})));
libcxx/test/std/containers/views/span.cons/range.pass.cpp
24

s/rU/U/, although if you just killed this entire comment block, that'd also be fine with me, personally.

47–50

test_from_range should return void, and these lines shouldn't assert.

65–66

Also worth checking:

static_assert(!std::is_constructible_v<std::span<const int>, std::vector<int>>); // non-borrowed rvalue
static_assert(!std::is_constructible_v<std::span<const int, 3>, std::vector<int>>); // non-borrowed rvalue

Try to arrange the lines so that they repeat in the same pattern as much as possible. E.g. you could improve the pattern by switching lines 60-61 with lines 62-63.

70–71

I waffle on this, but I think it'll be better to use the random_access_iterator from test_iterators.h instead of depending on deque. Also, technically, we can leave off the trailing && because is_constructible is just going to re-apply && on its side, anyway. E.g.

static_assert(std::is_constructible_v<std::span<int>, std::ranges::subrange<contiguous_iterator<int*>>>);  // contiguous borrowed rvalue
static_assert(!std::is_constructible_v<std::span<int>, std::ranges::subrange<random_access_iterator<int*>>>);  // non-contiguous borrowed rvalue
This revision now requires changes to proceed.Oct 1 2021, 4:47 PM
Quuxplusone added inline comments.Oct 2 2021, 7:40 PM
libcxx/test/std/containers/views/span.cons/iterator_len.verify.cpp
29

If you decide to keep a .verify.cpp test for span, then please add a test for

std::span<const int> sp = {0, 0};

Today we accept that (interpreting 0 as a null pointer constant); after this PR we should correctly reject it (as libstdc++ and MSVC already do).

Quuxplusone added inline comments.Oct 2 2021, 8:05 PM
libcxx/test/std/containers/views/span.cons/iterator_len.pass.cpp
22

#include <iterator> for std::data and std::size.
Generally, check the output of

./bin/llvm-lit -sv --param std=c++2b --param cxx_under_test=`pwd`/bin/clang++ --param enable_modules=True ../libcxx/test/std/containers/views/

(notice --param enable_modules=True) and fix the things it complains about, both in <span> and in the tests. buildkite will also test this, in the "Modules" build step; but the above magic incantation will let you do it locally.

jloser updated this revision to Diff 376893.Oct 4 2021, 7:34 AM
jloser marked 10 inline comments as done.

Address some of Arthur's feedback.

  • Remove unused type trait in span
  • Fix typos in a variety of places (e.g. s/extent/Extent, s/constructability/constructibility)
  • Rename iterator_iterator.{pass,verify}.cpp to iterator_sentinel.{pass,verify}.cpp
jloser added inline comments.Oct 4 2021, 7:34 AM
libcxx/include/span
70

For consistency with our synopsis, I'll use Extent. But do note the standard uses extent in the synopsis. I don't like differing just for preference with the implementation of how we use Extent everywhere, but I like being locally consistent within a neighborhood of code.

170–171

It's unused and removed now. Good call!

libcxx/test/std/containers/views/span.cons/iterator_len.verify.cpp
29

Added said test and we properly reject it.

jloser updated this revision to Diff 376897.Oct 4 2021, 7:51 AM

Address some of Arthur's feedback.

  • test_from_range should return void - cleans up some asserts in callers
  • Adjust tests a bit in range.pass.cpp to avoid dependency on deque
  • Reoganize some tests for clarity in range.pass.cpp
jloser marked 3 inline comments as done.Oct 4 2021, 7:52 AM
Quuxplusone added inline comments.Oct 4 2021, 7:54 AM
libcxx/include/span
70

Ah, yeah, I noticed the Standard's use of extent yesterday while working on https://quuxplusone.github.io/blog/2021/10/03/p2447-span-from-initializer-list/ . So it wasn't just a typo. I agree, I like consistency. But today I have no strong preference between the spellings: if we want to use extent consistently, or Extent consistently, I don't care.

233

This has the same problem as in the string_view PR: I don't think operator<= is always valid here. Please add a test for an iterator/sentinel pair that doesn't support <=.

The issue is not that __first <= __last might be false; the issue is that it might not be well-formed at all.

jloser marked an inline comment as done.Oct 4 2021, 8:27 AM
jloser added inline comments.
libcxx/include/span
233

The well-formedness is definitely a wrinkle. I'll add a test to expose this issue and remove the _LIBCPP_ASSERT.

Obviously, we could guard the _LIBCPP_ASSERT with requires to assure well-formedness of __first <= __last, but it's probably not worth it. I'd like to get some clarity in general on precondition checking in libc++ as I mentioned in the string_view PR. WDYT?

jloser updated this revision to Diff 377005.Oct 4 2021, 12:40 PM

Rebase to trigger CI

jloser updated this revision to Diff 377080.Oct 4 2021, 9:13 PM

Mark several other span tests as UNSUPPORTED: libcpp-has-no-incomplete-ranges

jloser updated this revision to Diff 377170.Oct 5 2021, 5:51 AM

Mark empty.pass.cpp as UNSUPPORTED: libcpp-has-no-incomplete-ranges

jloser updated this revision to Diff 377373.Oct 5 2021, 3:48 PM

XFAIL: AppleClang-12.0.0 a bunch of span tests.
Fix modules build for use of ranges::size in span

jloser updated this revision to Diff 377375.Oct 5 2021, 3:59 PM

Add test for constructing span from iterator/sentinel
Add sized_sentinel_wrapper to "test_iterators.h"

jloser updated this revision to Diff 377376.Oct 5 2021, 4:03 PM

[NFC] Spacing in sized_sentinel_wrapper's operator--

ldionne added inline comments.Oct 5 2021, 6:17 PM
libcxx/test/std/containers/views/span.cons/assign.pass.cpp
12

It just occurred to me that if you wanted, you could also replace all those XFAIL: apple-clang-12.0.0 by XFAIL: libcpp-no-concepts. IDK why I didn't think of that sooner. Feel free to do it or not, those are both temporary things anyway.

jloser updated this revision to Diff 377543.Oct 6 2021, 7:28 AM

Fix debug build

jloser marked 4 inline comments as done.Oct 6 2021, 7:40 AM
jloser added inline comments.
libcxx/test/std/containers/views/span.cons/assign.pass.cpp
12

Eh, I'll leave it as is for now. I'm hoping it won't live very long as we talked about since we're dropping support for AppleClang 12.0.0 shortly.

jloser marked an inline comment as done.Oct 6 2021, 7:42 AM

@Quuxplusone @ldionne this PR should be ready with CI passing on this revision. Feel free to submit follow-up PRs for cleaning up other unrelated (from the purpose of this PR) constructor's SFINAE with nullptr, etc.

Appreciate the thorough review!

jloser updated this revision to Diff 377616.Oct 6 2021, 11:21 AM

Rebase since D110718 landed

ldionne accepted this revision.Oct 8 2021, 8:34 AM

LGTM, please wait a couple hours for Arthur to say "no" if he still has stuff to say, but otherwise ship it!

Quuxplusone accepted this revision.Oct 8 2021, 10:15 AM
This revision is now accepted and ready to land.Oct 8 2021, 10:15 AM
ldionne added inline comments.Mar 14 2022, 9:43 AM
libcxx/test/std/containers/views/span.cons/range.pass.cpp
31

I know this has been landed for a while, but in retrospect this test doesn't seem sufficient to me. I stumbled upon it doing something else and noticed. We are actually testing the array constructor of std::span, unless I am missing something.

Also, ideally, we would add a runtime test instead of each test below where we check that std::span is positively constructible from something.

@jloser Would you be interested in revisiting those? If not, I'll make a note to get to it eventually.

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 9:43 AM
libcxx/test/std/containers/views/span.cons/ptr_ptr.fail.cpp