[PATCH] D49338: Implement <span> - P0122R7

[Marshall Clow via Phabricator via cfe-commits]

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

I missed the container constructors being `noexcept`, and the asserts on `operator[]` and `operator()` are half done (stupid signed indicies).

I'll wait for other feedback before doing those bits.



================
Comment at: include/span:320
+    constexpr span<element_type, dynamic_extent>
+    inline _LIBCPP_INLINE_VISIBILITY
+       subspan(index_type __offset, index_type __count = dynamic_extent) const noexcept
----------------
ldionne wrote:
> I think we generally put those annotations before the return type. Consider doing so here and below for consistency.
I'm not sure where you're referring to "and below". We already do so for the other `subspan`.
But definitely here.



================
Comment at: include/span:335-336
+
+    _LIBCPP_INLINE_VISIBILITY constexpr reference operator[](index_type __idx) const noexcept { return __data[__idx]; }
+    _LIBCPP_INLINE_VISIBILITY constexpr reference operator()(index_type __idx) const noexcept { return __data[__idx]; }
+    _LIBCPP_INLINE_VISIBILITY constexpr pointer data()                         const noexcept { return __data; }
----------------
ldionne wrote:
> Those two could have a `_LIBCPP_ASSERT(__idx >= 0 && __idx < size())`. Is it a conscious choice not to put one?
D'oh! only half done!


https://reviews.llvm.org/D49338