ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed.
My comments are based off of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0122r7.pdf. ================ Comment at: include/span:189 +struct __is_span_compatible_container<_Tp, _ElementType, + void_t< + // is not a specialization of span ---------------- 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))`. ================ Comment at: include/span:199 + typename enable_if< + __is_span_compatible_ptr< + remove_pointer_t<decltype(data(declval<_Tp &>()))>, ---------------- I would personally inline this condition for readability. ================ Comment at: include/span:217 + using pointer = _Tp *; + using const_pointer = const _Tp *; // not in standard + using reference = _Tp &; ---------------- Why are we providing them if they are not in the standard? ================ Comment at: include/span:235 + + _LIBCPP_INLINE_VISIBILITY constexpr span(pointer __ptr, index_type __count) : __data{__ptr} + { (void)__count; _LIBCPP_ASSERT(_Extent == __count, "size mismatch in span's constructor (ptr, len)"); } ---------------- 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? ================ Comment at: include/span:240 + + _LIBCPP_INLINE_VISIBILITY constexpr span(element_type (&__arr)[_Extent]) : __data{__arr} {} + _LIBCPP_INLINE_VISIBILITY constexpr span( array<value_type, _Extent>& __arr) : __data{__arr.data()} {} ---------------- 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. ================ Comment at: include/span:254 + constexpr span(const _Container& __c, + const enable_if_t<__is_span_compatible_container<const _Container, _Tp>::value, nullptr_t> = nullptr) + : __data{_VSTD::data(__c)} ---------------- 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? ================ Comment at: include/span:261 + constexpr span(const span<_OtherElementType, _Extent>& __other, + const enable_if_t< + is_convertible_v<_OtherElementType(*)[], element_type (*)[]>, ---------------- `const enable_if`? Applies to the constructor below too. ================ Comment at: include/span:263 + is_convertible_v<_OtherElementType(*)[], element_type (*)[]>, + nullptr_t> = nullptr) + : __data{__other.data()} {} ---------------- Missing `noexcept` according to the paper. Applies to the constructor below too. ================ Comment at: include/span:275 + +// ~span() noexcept = default; + ---------------- Why is this commented out? ================ Comment at: include/span:282 + static_assert(_Count >= 0); + _LIBCPP_ASSERT(_Count <= size(), "Count out of range in span::first()"); + return {data(), _Count}; ---------------- This can be turned into a `static_assert` since we know the extent of the span at compile-time. ================ Comment at: include/span:291 + static_assert(_Count >= 0); + _LIBCPP_ASSERT(_Count <= size(), "Count out of range in span::last()"); + return {data() + size() - _Count, _Count}; ---------------- This can be turned into a `static_assert` since we know the extent of the span at compile-time. ================ Comment at: include/span:320 + constexpr span<element_type, dynamic_extent> + inline _LIBCPP_INLINE_VISIBILITY + subspan(index_type __offset, index_type __count = dynamic_extent) const noexcept ---------------- I think we generally put those annotations before the return type. Consider doing so here and below for consistency. ================ Comment at: include/span:335-336 + + _LIBCPP_INLINE_VISIBILITY constexpr reference operator[](index_type __idx) const noexcept { return __data[__idx]; } + _LIBCPP_INLINE_VISIBILITY constexpr reference operator()(index_type __idx) const noexcept { return __data[__idx]; } + _LIBCPP_INLINE_VISIBILITY constexpr pointer data() const noexcept { return __data; } ---------------- Those two could have a `_LIBCPP_ASSERT(__idx >= 0 && __idx < size())`. Is it a conscious choice not to put one? ================ Comment at: include/span:351 + { + pointer __p = __data; + __data = __other.__data; ---------------- Just curious -- why not use `_VSTD::swap(__data, __other.__data)`? This would avoid any potential for a stupid logic error to sneak up. ================ Comment at: include/span:356-359 + _LIBCPP_INLINE_VISIBILITY span<const byte, _Extent * sizeof(element_type)> __as_bytes() const noexcept + { return {reinterpret_cast<const byte *>(data()), size_bytes()}; } + + _LIBCPP_INLINE_VISIBILITY span<byte, _Extent * sizeof(element_type)> __as_writeable_bytes() const noexcept ---------------- It looks like neither `as_bytes` nor `as_writeable_bytes` is marked `const` in the paper. Why are we deviating? ================ Comment at: include/span:415 + constexpr span( _Container& __c, + const enable_if_t<__is_span_compatible_container<_Container, _Tp>::value, nullptr_t> = nullptr) + : __data{_VSTD::data(__c)}, __size{(index_type) _VSTD::size(__c)} {} ---------------- Same comments as for the specialization with a static `_Extent`. ================ Comment at: include/span:531 + operator==(const span<_Tp1, _Extent1>& __lhs, const span<_Tp2, _Extent2>& __rhs) + { return equal(__lhs.begin(), __lhs.end(), __rhs.begin(), __rhs.end()); } + ---------------- 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`. https://reviews.llvm.org/D49338 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
