[PATCH] D49338: Implement <span> - P0122R7
Marshall Clow via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 23 13:21:48 PDT 2018
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
More information about the cfe-commits
mailing list