[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
Wed Sep 29 09:49:52 PDT 2021
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
More information about the libcxx-commits
mailing list