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