Implement https://wg21.link/p1394 which allows span to be constructible
from any contiguous forwarding-range that has a compatible element type.
Details
- Reviewers
ldionne • Quuxplusone cjdb Mordante mclow.lists - Group Reviewers
Restricted Project - Commits
- rG3a208c68942e: [libc++] Implement P1394r4 for span: range constructor
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 126598 Build 183941: arc lint + arc unit
Event Timeline
libcxx/include/span | ||
---|---|---|
69–71 | This ctor should also be conditionally explicit, according to https://eel.is/c++draft/span.cons | |
179–183 | If you're updating these, might as well make them all use e.g. | |
196 | 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. | |
438 | ADL-proof: _VSTD::to_address, _VSTD::distance. Ditto lines 236, 248. 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). | |
462–463 | 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). | |
591–592 | 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. | |
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? |
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
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 | ||
---|---|---|
195 | 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. |
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.
libcxx/include/span | ||
---|---|---|
196 | 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<...>. |
libcxx/include/span | ||
---|---|---|
195 | 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. |
- 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
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. |
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). |
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. |
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. |
Collapse .compile.pass.cpp into their respective .pass.cpp files
Try to fix #ifdefs to make CI happy
@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 | ||
---|---|---|
204 | Nit: s/>> (/>>(/ for consistency with _ElementType(*)[]. | |
235–260 | 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 | ||
---|---|---|
465–471 | 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!) |
Enable range constructor in C++20 mode, not > C++20 which should fix CI
Fix some clang-format mishaps with cuddling of >> vs > >.
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!.
libcxx/include/span | ||
---|---|---|
465–471 | I'd vote let's just do that consistently throughout this file in a follow-up PR if that's OK with you. |
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 | ||
---|---|---|
195 | 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. | |
239 | 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. |
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 :(
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.
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/ | |
179–183 | 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. | |
196 | 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). | |
438 |
| |
libcxx/test/std/containers/views/span.cons/iterator_iterator.pass.cpp | ||
16 | s/extent/Extent/g | |
33 | s/tability/tibility/ | |
libcxx/test/std/containers/views/span.cons/iterator_iterator.verify.cpp | ||
21 | s/extent/Extent/ | |
29 | 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 | ||
23 | s/rU/U/, although if you just killed this entire comment block, that'd also be fine with me, personally. | |
46–49 | test_from_range should return void, and these lines shouldn't assert. | |
64–65 | 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. | |
69–70 | 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 |
libcxx/test/std/containers/views/span.cons/iterator_len.verify.cpp | ||
---|---|---|
28 | 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). |
libcxx/test/std/containers/views/span.cons/iterator_len.pass.cpp | ||
---|---|---|
21 | #include <iterator> for std::data and std::size. ./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. |
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
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. | |
179–183 | It's unused and removed now. Good call! | |
libcxx/test/std/containers/views/span.cons/iterator_len.verify.cpp | ||
28 | Added said test and we properly reject it. |
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
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. | |
255 | 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. |
libcxx/include/span | ||
---|---|---|
255 | 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? |
XFAIL: AppleClang-12.0.0 a bunch of span tests.
Fix modules build for use of ranges::size in span
Add test for constructing span from iterator/sentinel
Add sized_sentinel_wrapper to "test_iterators.h"
libcxx/test/std/containers/views/span.cons/assign.pass.cpp | ||
---|---|---|
12 ↗ | (On Diff #377376) | 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. |
libcxx/test/std/containers/views/span.cons/assign.pass.cpp | ||
---|---|---|
12 ↗ | (On Diff #377376) | 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. |
@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!
LGTM, please wait a couple hours for Arthur to say "no" if he still has stuff to say, but otherwise ship it!
libcxx/test/std/containers/views/span.cons/range.pass.cpp | ||
---|---|---|
32 | 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. |
s/extent/Extent/