[libcxx-commits] [PATCH] D109475: [libc++] Simplify span specializations
Joe Loser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Sep 9 15:28:42 PDT 2021
jloser added inline comments.
================
Comment at: libcxx/include/span:439
pointer __data;
-
-};
-
-
-template <typename _Tp>
-class _LIBCPP_TEMPLATE_VIS span<_Tp, dynamic_extent> {
-private:
-
-public:
-// constants and types
- using element_type = _Tp;
- using value_type = remove_cv_t<_Tp>;
- using size_type = size_t;
- using difference_type = ptrdiff_t;
- using pointer = _Tp *;
- using const_pointer = const _Tp *;
- using reference = _Tp &;
- using const_reference = const _Tp &;
-#if (_LIBCPP_DEBUG_LEVEL == 2) || defined(_LIBCPP_ABI_SPAN_POINTER_ITERATORS)
- using iterator = pointer;
-#else
- using iterator = __wrap_iter<pointer>;
-#endif
- using reverse_iterator = _VSTD::reverse_iterator<iterator>;
-
- static constexpr size_type extent = dynamic_extent;
-
-// [span.cons], span constructors, copy, assignment, and destructor
- _LIBCPP_INLINE_VISIBILITY constexpr span() noexcept : __data{nullptr}, __size{0} {}
-
- constexpr span (const span&) noexcept = default;
- constexpr span& operator=(const span&) noexcept = default;
-
- _LIBCPP_INLINE_VISIBILITY constexpr span(pointer __ptr, size_type __count) : __data{__ptr}, __size{__count} {}
- _LIBCPP_INLINE_VISIBILITY constexpr span(pointer __f, pointer __l) : __data{__f}, __size{static_cast<size_t>(distance(__f, __l))} {}
-
- template <size_t _Sz>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(element_type (&__arr)[_Sz]) noexcept : __data{__arr}, __size{_Sz} {}
-
- template <class _OtherElementType, size_t _Sz,
- enable_if_t<is_convertible_v<_OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(array<_OtherElementType, _Sz>& __arr) noexcept : __data{__arr.data()}, __size{_Sz} {}
-
- template <class _OtherElementType, size_t _Sz,
- enable_if_t<is_convertible_v<const _OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(const array<_OtherElementType, _Sz>& __arr) noexcept : __data{__arr.data()}, __size{_Sz} {}
-
- template <class _Container>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span( _Container& __c,
- enable_if_t<__is_span_compatible_container<_Container, _Tp>::value, nullptr_t> = nullptr)
- : __data{_VSTD::data(__c)}, __size{(size_type) _VSTD::size(__c)} {}
-
- template <class _Container>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(const _Container& __c,
- enable_if_t<__is_span_compatible_container<const _Container, _Tp>::value, nullptr_t> = nullptr)
- : __data{_VSTD::data(__c)}, __size{(size_type) _VSTD::size(__c)} {}
-
-
- template <class _OtherElementType, size_t _OtherExtent>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(const span<_OtherElementType, _OtherExtent>& __other,
- enable_if_t<
- is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
- nullptr_t> = nullptr) noexcept
- : __data{__other.data()}, __size{__other.size()} {}
-
-// ~span() noexcept = default;
-
- template <size_t _Count>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span<element_type, _Count> first() const noexcept
- {
- _LIBCPP_ASSERT(_Count <= size(), "Count out of range in span::first()");
- return span<element_type, _Count>{data(), _Count};
- }
-
- template <size_t _Count>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span<element_type, _Count> last() const noexcept
- {
- _LIBCPP_ASSERT(_Count <= size(), "Count out of range in span::last()");
- return span<element_type, _Count>{data() + size() - _Count, _Count};
- }
-
- _LIBCPP_INLINE_VISIBILITY
- constexpr span<element_type, dynamic_extent> first(size_type __count) const noexcept
- {
- _LIBCPP_ASSERT(__count <= size(), "Count out of range in span::first(count)");
- return {data(), __count};
- }
-
- _LIBCPP_INLINE_VISIBILITY
- constexpr span<element_type, dynamic_extent> last (size_type __count) const noexcept
- {
- _LIBCPP_ASSERT(__count <= size(), "Count out of range in span::last(count)");
- return {data() + size() - __count, __count};
- }
-
- template <size_t _Offset, size_t _Count = dynamic_extent>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span<element_type, _Count> subspan() const noexcept
- {
- _LIBCPP_ASSERT(_Offset <= size(), "Offset out of range in span::subspan()");
- _LIBCPP_ASSERT(_Count == dynamic_extent || _Count <= size() - _Offset, "Offset + count out of range in span::subspan()");
- return span<element_type, _Count>{data() + _Offset, _Count == dynamic_extent ? size() - _Offset : _Count};
- }
-
- constexpr span<element_type, dynamic_extent>
- _LIBCPP_INLINE_VISIBILITY
- subspan(size_type __offset, size_type __count = dynamic_extent) const noexcept
- {
- _LIBCPP_ASSERT(__offset <= size(), "Offset out of range in span::subspan(offset, count)");
- _LIBCPP_ASSERT(__count <= size() || __count == dynamic_extent, "count out of range in span::subspan(offset, count)");
- if (__count == dynamic_extent)
- return {data() + __offset, size() - __offset};
- _LIBCPP_ASSERT(__count <= size() - __offset, "Offset + count out of range in span::subspan(offset, count)");
- return {data() + __offset, __count};
- }
-
- _LIBCPP_INLINE_VISIBILITY constexpr size_type size() const noexcept { return __size; }
- _LIBCPP_INLINE_VISIBILITY constexpr size_type size_bytes() const noexcept { return __size * sizeof(element_type); }
- _LIBCPP_INLINE_VISIBILITY constexpr bool empty() const noexcept { return __size == 0; }
-
- _LIBCPP_INLINE_VISIBILITY constexpr reference operator[](size_type __idx) const noexcept
- {
- _LIBCPP_ASSERT(__idx < size(), "span<T>[] index out of bounds");
- return __data[__idx];
- }
-
- _LIBCPP_INLINE_VISIBILITY constexpr reference front() const noexcept
- {
- _LIBCPP_ASSERT(!empty(), "span<T>[].front() on empty span");
- return __data[0];
- }
-
- _LIBCPP_INLINE_VISIBILITY constexpr reference back() const noexcept
- {
- _LIBCPP_ASSERT(!empty(), "span<T>[].back() on empty span");
- return __data[size()-1];
- }
-
-
- _LIBCPP_INLINE_VISIBILITY constexpr pointer data() const noexcept { return __data; }
-
-// [span.iter], span iterator support
- _LIBCPP_INLINE_VISIBILITY constexpr iterator begin() const noexcept { return iterator(data()); }
- _LIBCPP_INLINE_VISIBILITY constexpr iterator end() const noexcept { return iterator(data() + size()); }
- _LIBCPP_INLINE_VISIBILITY constexpr reverse_iterator rbegin() const noexcept { return reverse_iterator(end()); }
- _LIBCPP_INLINE_VISIBILITY constexpr reverse_iterator rend() const noexcept { return reverse_iterator(begin()); }
-
- _LIBCPP_INLINE_VISIBILITY span<const byte, dynamic_extent> __as_bytes() const noexcept
- { return {reinterpret_cast<const byte *>(data()), size_bytes()}; }
-
- _LIBCPP_INLINE_VISIBILITY span<byte, dynamic_extent> __as_writable_bytes() const noexcept
- { return {reinterpret_cast<byte *>(data()), size_bytes()}; }
-
-private:
- pointer __data;
- size_type __size;
+ [[no_unique_address]] __extent_storage<_Extent> __extent;
};
----------------
Quuxplusone wrote:
> jloser wrote:
> > curdeius wrote:
> > > To avoid an ABI break in the case of static extent, you suppose that `[[no_unique_address]]` will be honoured by the compiler, right?
> > > Because otherwise, the size of `span<T, N>` (`N != dynamic_extent`) would change.
> > > Unless I'm mistaken, MSVC doesn't honour this attribute unless forced (cf. https://devblogs.microsoft.com/cppblog/msvc-cpp20-and-the-std-cpp20-switch/#msvc-extensions-and-abi).
> > > Not sure whether that's a problem though.
> > Right. We can't have an ABI break here and that was my intent with use of `[[no_unique_address]]`. The blog post you linked and the [[ https://github.com/microsoft/STL/issues/1364 | GitHub issue it mentions ]] has me a bit worried.
> >
> > With that being said, I'm going to just use a base class to avoid an ABI break. I would prefer to use `[[no_unique_address]]`, but playing things safe is my preference here.
> >
> > I will note that we unconditionally use `[[no_unique_address]]` in several of `ranges` things that are C++20-only; it's slightly different than `span` though as `span` has been shipping for a few years now.
> Maybe I'm missing something, but isn't the base-class approach super obviously an ABI break? The old libc++ code stored "pointer, then size" and this PR's code stores "size, then pointer".
>
> Also, storing members in a dependent base class means a lot more `this->` noise in member functions.
>
> If we're just worried about the difficulty of dealing with two very similar copies of the same code, how about we just move the specializations into `__span/static_extent.h` and `__span/dynamic_extent.h`, and then try to minimize the `diff` between them?
>
> Or, do nothing. I'm still unclear on what is the benefit here or how this relates to enable_if or whatever was being talked about earlier.
No, you're right regarding the ABI break. Having the base class means we'll change the layout which is an obvious ABI break and we can't do that.
My initial worry was indeed in dealing with two very similar copies of the code. I'm not thrilled with how similar the two specializations are, but I think I'll implement P1394 and see what the divergence looks like then and revisit this. We could conditionally define the macro for `no_unique_address` to workaround MSVC to avoid the ABI break there and go that approach.
In any case, I'm going to abandon this review for now and think about this some more - may revisit after P1394.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109475/new/
https://reviews.llvm.org/D109475
More information about the libcxx-commits
mailing list