[libcxx-commits] [PATCH] D110503: [libc++] Implement P1394r4 for span: range constructor

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

In D110503#3030310 <https://reviews.llvm.org/D110503#3030310>, @jloser wrote:

> @ldionne @Quuxplusone can you help me understand why a few `static_assert`s 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.)



================
Comment at: libcxx/include/span:204
+  !is_array_v<remove_cvref_t<_Range>> &&
+  is_convertible_v<remove_reference_t<ranges::range_reference_t<_Range>> (*)[], _ElementType(*)[]>;
+#endif
----------------
Nit: `s/>> (/>>(/` for consistency with `_ElementType(*)[]`.


================
Comment at: libcxx/include/span:235-238
+    template <class _It,
+              enable_if_t<contiguous_iterator<_It> &&
+                              is_convertible_v<remove_reference_t<iter_reference_t<_It> > (*)[], element_type (*)[]>,
+                          nullptr_t> = nullptr>
----------------
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`.)


================
Comment at: libcxx/include/span:455-461
     template <class _OtherElementType, size_t _OtherExtent>
     _LIBCPP_INLINE_VISIBILITY
         constexpr span(const span<_OtherElementType, _OtherExtent>& __other,
                        enable_if_t<
                           is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
                           nullptr_t> = nullptr) noexcept
         : __data{__other.data()}, __size{__other.size()} {}
----------------
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`!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110503/new/

https://reviews.llvm.org/D110503