[libcxx-commits] [PATCH] D110503: [libc++] Implement P1394r4 for span: range constructor
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Oct 1 16:47:52 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.
Mostly nits, but "this helper trait is now completely unused" is significant enough that I think we'd benefit from one more round.
================
Comment at: libcxx/include/span:70
+ template<class R>
+ constexpr explicit(extent != dynamic_extent) span(R&& r);
constexpr span(const span& other) noexcept = default;
----------------
`s/extent/Extent/`
================
Comment at: libcxx/include/span:179-183
+ typename enable_if<!__is_std_span<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
// is not a specialization of array
- typename enable_if<!__is_std_array<_Tp>::value, nullptr_t>::type,
+ typename enable_if<!__is_std_array<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
// is_array_v<Container> is false,
typename enable_if<!is_array_v<_Tp>, nullptr_t>::type,
----------------
Quuxplusone wrote:
> 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>,`
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.
================
Comment at: libcxx/include/span:196
+template <class _Range, class _ElementType>
+inline constexpr bool __is_range_compatible =
+ ranges::contiguous_range<_Range> &&
----------------
jloser wrote:
> Quuxplusone wrote:
> > 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.
> 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<...>`.
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.
================
Comment at: libcxx/include/span:429
+ _LIBCPP_INLINE_VISIBILITY constexpr span(_It __first, _End __last)
+ : __data{to_address(__first)}, __size{static_cast<size_t>(distance(__first, __last))} {}
----------------
Quuxplusone wrote:
> 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).
- 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`.
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_iterator.pass.cpp:16
+// Requires: [first, last) shall be a valid range.
+// If extent is not equal to dynamic_extent, then last - first shall be equal to extent.
+//
----------------
s/extent/Extent/g
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_iterator.pass.cpp:33
+template <size_t Extent>
+constexpr void test_constructability() {
+ static_assert(std::is_constructible_v<std::span<int, Extent>, int*, int*>);
----------------
s/tability/tibility/
and likewise in at least one other file
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_iterator.verify.cpp:21
+
+template<class T, size_t extent>
+std::span<T, extent> createImplicitSpan(T* first, T* last) {
----------------
s/extent/Extent/
I'm beginning to suspect some automated process was responsible for lowercasing all of these `Extent`s. :)
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_iterator.verify.cpp:29
+ int arr[] = {1, 2, 3};
+ createImplicitSpan<int, 1>(std::begin(arr), std::next(std::begin(arr)));
+
----------------
`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})));
```
================
Comment at: libcxx/test/std/containers/views/span.cons/range.pass.cpp:23
+// — is_array_v<remove_cvref_t<R>> is false,
+// - is_convertible_v<rU(*)[], element_type(*)[] is true
+
----------------
`s/rU/U/`, although if you just killed this entire comment block, that'd also be fine with me, personally.
================
Comment at: libcxx/test/std/containers/views/span.cons/range.pass.cpp:46-49
+ assert((test_from_range<int, std::dynamic_extent>()));
+ assert((test_from_range<int, 3>()));
+ assert((test_from_range<A, std::dynamic_extent>()));
+ assert((test_from_range<A, 3>()));
----------------
`test_from_range` should return void, and these lines shouldn't `assert`.
================
Comment at: libcxx/test/std/containers/views/span.cons/range.pass.cpp:64-65
+static_assert(std::is_constructible_v<std::span<const int, 3>, 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, 3>, std::vector<int>&&>); // non-borrowed rvalue
+static_assert(!std::is_constructible_v<std::span<int>, std::deque<int>&>); // non-contiguous lvalue
----------------
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.
================
Comment at: libcxx/test/std/containers/views/span.cons/range.pass.cpp:69-70
+static_assert(!std::is_constructible_v<
+ std::span<int>, std::ranges::subrange<std::deque<int>::iterator,
+ std::deque<int>::iterator>&&>); // non-contiguous borrowed rvalue
+static_assert(!std::is_constructible_v<
----------------
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
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110503/new/
https://reviews.llvm.org/D110503
More information about the libcxx-commits
mailing list