[PATCH] D49338: Implement <span> - P0122R7

[Louis Dionne via Phabricator via cfe-commits]

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