This is an archive of the discontinued LLVM Phabricator instance.

Implement <span> - P0122R7
ClosedPublic

Authored by mclow.lists on Jul 13 2018, 10:47 PM.

Details

Summary

Implement the header <span> for C++20.

This is a very large patch, but almost 90% of the code is tests. (4400 lines of 5000)
The paper can be found here https://wg21.link/P0122

Diff Detail

Event Timeline

mclow.lists created this revision.Jul 13 2018, 10:47 PM
mclow.lists added inline comments.Jul 13 2018, 11:02 PM
include/span
237

These naked assert calls need to be _LIBCPP_ASSERT

Need to add an entry to include/CMakeLists.txt as well.

STL_MSFT added inline comments.Jul 16 2018, 12:49 PM
test/std/containers/views/span.comparison/op.eq.pass.cpp
24

The comparison tests appear to be unnecessarily including <string>.

test/std/containers/views/span.cons/assign.pass.cpp
26

Technically need <utility> for declval.

74

span is C++20, so you can use C++14 extended constexpr's for-loops. Just write a helper function.

test/std/containers/views/span.cons/container.fail.cpp
45

This file is inconsistent about left vs right const (see line immediately above).

test/std/containers/views/span.cons/deduct.pass.cpp
46

Technically need <type_traits> for is_same_v.

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

This return appears to be spurious.

test/std/containers/views/span.cons/span.pass.cpp
97

Spurious return occurs in multiple files.

test/std/containers/views/span.iterators/begin.pass.cpp
32

Using bitwise &= on a bool isn't very cromulent.

test/std/containers/views/span.iterators/rbegin.pass.cpp
116

static_cast would be nicer than C cast.

test/std/containers/views/span.objectrep/as_bytes.pass.cpp
32

This is a C++17 terse static assert unlike your others. Should you consistently use this form, or the other one?

test/std/containers/views/span.objectrep/as_writeable_bytes.pass.cpp
43

static_cast?

test/std/containers/views/span.sub/last.pass.cpp
45

Need <algorithm>.

test/std/containers/views/span.sub/subspan.pass.cpp
46

Also need <algorithm>, presumably occurs in more files.

test/std/containers/views/types.pass.cpp
42

Need <iterator>.

Address (all but one of) STL's comments

mclow.lists marked 13 inline comments as done.Jul 17 2018, 7:12 AM
mclow.lists marked an inline comment as done.
ldionne requested changes to this revision.Jul 23 2018, 12:55 PM
include/span
190

You seem to be missing the following condition from the paper: is_array_v<Container> is false. Are you omitting it because the array overloads would take precedence over this constructor if an array were passed? If so, I think there's a bug since you could be passing an array of incorrect size and this constructor would kick in. The overload for ElementType(*)[N] would be discarded because of a mismatched N, but this one wouldn't (I think).

If I'm right, this would turn what should be a compilation failure into a runtime error with the _LIBCPP_ASSERT(_Extent == _VSTD::size(__c)).

200

I would personally inline this condition for readability.

218

Why are we providing them if they are not in the standard?

236

Those functions are specified to throw nothing in the standard. Do we want to add noexcept as a QOI thing? What's our usual stance on this?

241

We're missing noexcept on these 3. They are specified as noexcept in the standard so it's not only QOI. Also, consider adding a test to catch this mistake.

255

For both of these Container constructors, the paper expresses the SFINAE conditions based on Container, not on Container in one case and Container const in the other, which is what you're doing.

This is actually a bug in the paper, because this will make code like this compile:

std::vector<int> const v;
std::span<int, 10> s(v);

Instead, this should be a compiler error because we're clearly not const-correct here, initializing a span-over-non-const from a const vector. Example: https://wandbox.org/permlink/kYCui3o0LEGRQ67x

This happens because we're discarding the constness of the _Container template parameter if we stick 100% to the wording of the paper. Should this be a DR?

262

const enable_if? Applies to the constructor below too.

264

Missing noexcept according to the paper. Applies to the constructor below too.

276

Why is this commented out?

283

This can be turned into a static_assert since we know the extent of the span at compile-time.

292

This can be turned into a static_assert since we know the extent of the span at compile-time.

321

I think we generally put those annotations before the return type. Consider doing so here and below for consistency.

336–337

Those two could have a _LIBCPP_ASSERT(__idx >= 0 && __idx < size()). Is it a conscious choice not to put one?

352

Just curious -- why not use _VSTD::swap(__data, __other.__data)? This would avoid any potential for a stupid logic error to sneak up.

357–360

It looks like neither as_bytes nor as_writeable_bytes is marked const in the paper. Why are we deviating?

416

Same comments as for the specialization with a static _Extent.

532

It's kind of crazy those are not constrained in any way, but that's what the paper says. I would expect some constraint based on whether we can compare _Tp1 and _Tp2`.

This revision now requires changes to proceed.Jul 23 2018, 12:55 PM
mclow.lists marked 5 inline comments as done.Jul 23 2018, 1:21 PM

I think I've answered all of Louis' questions that don't require code changes.
New patch will be coming soon.

include/span
218

Because (a) they're useful (see the definition of const_iterator below, and (b) I (and STL, who wrote the final version of the span paper, believe that not having them was just an oversight.

236

Yes. The standard follows the "Lakos rule", which says that narrow contract functions should not be marked noexcept, because goodness knows what might happen if you fail to fulfill the preconditions.

Library implementors have the freedom to mark things as noexcept (and libc++ does - see vector::operator[] for example).

255

Yes, this should be an LWG issue.

276

This is commented out because of a clang bug https://bugs.llvm.org/show_bug.cgi?id=38143, where copy constructor will become non-constexpr. Once this fixed, we can un-comment this line (though it should make no difference if it is present or not).

352

Not to have to include <utility>? (It may get included anyway)

357–360

In N4762, they're marked as non-member functions that take span<whatever> by value. This implementation just turns around and calls a member function (with an unpronounceable name) to do the work. The standard never mentions __as_bytes or __as_writeable_bytes.

532

In my prototype implementation, they were constrained. But LEWG decided not to go there (which is the same approach taken in https://wg21.link/P0805 ). There needs to be another paper written about constraining container comparisons.

It also applies to homogenous comparisons. Consider vector<complex<double>> Two of them can't be compared (less than, etc), because complex<double> doesn't have a operator<

mclow.lists marked 5 inline comments as done.

Address Louis' comments.

mclow.lists marked 13 inline comments as done.Jul 23 2018, 2:18 PM

I missed the container constructors being noexcept, and the asserts on operator[] and operator() are half done (stupid signed indicies).

I'll wait for other feedback before doing those bits.

include/span
321

I'm not sure where you're referring to "and below". We already do so for the other subspan.
But definitely here.

336–337

D'oh! only half done!

mclow.lists marked an inline comment as done.Jul 23 2018, 2:25 PM

I missed the container constructors being noexcept

Never mind; they're not noexcept (and they should not be)

ldionne added inline comments.Jul 23 2018, 2:45 PM
include/span
218

I sent you a LWG issue email.

255

Ugh, actually, I am wrong! The paper is fine and you implemented the paper correctly. The paper says

Remarks: These constructors shall not participate in overload resolution unless:
— `Container` is not a specialization of span,
— `Container` is not a specialization of array,
— `is_array_v<Container>` is `false`,
— `data(cont)` and `size(cont)` are both well-formed, and
— `remove_pointer_t<decltype(data(cont))>(*)[]` is convertible to `ElementType(*)[]`.

since cont has the right constness, the paper properly describes what should happen. My mistake.

276

What! Thanks.

352

<array> already includes <utility>. Your choice, I don't mind.

357–360

I completely missed the leading underscores on those methods, along with the non-member functions. Thanks, this is not an issue.

532

Thanks for the information.

Fix the assertions in the indexing operators.

Also change a bunch of the tests to use ASSERT_SAME_TYPE and ASSERT_NOEXCEPT. No functional change there, just better names.

ldionne accepted this revision.Jul 23 2018, 4:42 PM

LGTM, with a suggestion for adding one test.

include/span
190

Consider adding a test for this.

This revision is now accepted and ready to land.Jul 23 2018, 4:42 PM
mclow.lists closed this revision.Jul 26 2018, 6:50 PM

Landed as r337804.