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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
