[PATCH] D49338: Implement <span> - P0122R7

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.


================
Comment at: include/span:217
+    using pointer                = _Tp *;
+    using const_pointer          = const _Tp *; // not in standard
+    using reference              = _Tp &;
----------------
mclow.lists wrote:
> 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.
> 
I sent you a LWG issue email.


================
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)}
----------------
mclow.lists wrote:
> 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.
Ugh, actually, I am wrong! The paper is fine and you implemented the paper correctly. The paper says

```
Remarks: These constructors shall not participate in overload resolution unless:
— `Container` is not a specialization of span,
— `Container` is not a specialization of array,
— `is_array_v<Container>` is `false`,
— `data(cont)` and `size(cont)` are both well-formed, and
— `remove_pointer_t<decltype(data(cont))>(*)[]` is convertible to `ElementType(*)[]`.
```

since `cont` has the right constness, the paper properly describes what should happen. My mistake.



================
Comment at: include/span:275
+
+//  ~span() noexcept = default;
+
----------------
mclow.lists wrote:
> 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).
> 
What! Thanks.


================
Comment at: include/span:351
+    {
+        pointer __p = __data;
+        __data = __other.__data;
----------------
mclow.lists wrote:
> 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)
`<array>` already includes `<utility>`. Your choice, I don't mind.


================
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
----------------
mclow.lists wrote:
> 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`.
> 
> 
> 
> 
> 
I completely missed the leading underscores on those methods, along with the non-member functions. Thanks, this is not an issue.


================
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()); }
+
----------------
mclow.lists wrote:
> 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<`
Thanks for the information.


https://reviews.llvm.org/D49338