[PATCH] D49338: Implement <span> - P0122R7

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 5 inline comments as done.
mclow.lists added a comment.

I think I've answered all of Louis' questions that don't require code changes.
New patch will be coming soon.



================
Comment at: include/span:217
+    using pointer                = _Tp *;
+    using const_pointer          = const _Tp *; // not in standard
+    using reference              = _Tp &;
----------------
ldionne wrote:
> Why are we providing them if they are not in the standard?
Because (a) they're useful (see the definition of `const_iterator` below, and (b) I (and STL, who wrote the final version of the `span` paper, believe that not having them was just an oversight.



================
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)"); }
----------------
ldionne wrote:
> 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?
Yes. The standard follows the "Lakos rule", which says that narrow contract functions should not be marked `noexcept`,  because goodness knows what might happen if you fail to fulfill the preconditions.

Library implementors have the freedom to mark things as `noexcept` (and libc++ does - see `vector::operator[]` for example).




================
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)}
----------------
ldionne wrote:
> 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?
Yes, this should be an LWG issue.


================
Comment at: include/span:275
+
+//  ~span() noexcept = default;
+
----------------
ldionne wrote:
> Why is this commented out?
This is commented out because of a clang bug https://bugs.llvm.org/show_bug.cgi?id=38143, where copy constructor will become non-constexpr.  Once this fixed, we can un-comment this line (though it should make no difference if it is present or not).



================
Comment at: include/span:351
+    {
+        pointer __p = __data;
+        __data = __other.__data;
----------------
ldionne wrote:
> Just curious -- why not use `_VSTD::swap(__data, __other.__data)`? This would avoid any potential for a stupid logic error to sneak up.
Not to have to include `<utility>`? (It may get included anyway)


================
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
----------------
ldionne wrote:
> It looks like neither `as_bytes` nor `as_writeable_bytes` is marked `const` in the paper. Why are we deviating?
In N4762, they're marked as non-member functions that take `span<whatever>` by value. This implementation just turns around and calls a member function (with an unpronounceable name) to do the work.  The standard never mentions `__as_bytes` or `__as_writeable_bytes`.







================
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()); }
+
----------------
ldionne wrote:
> 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`.
In my prototype implementation, they were constrained. But LEWG decided not to go there (which is the same approach taken in https://wg21.link/P0805 ). There needs to be another paper written about constraining container comparisons.

It also applies to homogenous comparisons. Consider `vector<complex<double>>`  Two of them can't be compared (less than, etc), because `complex<double>` doesn't have a `operator<`


https://reviews.llvm.org/D49338