[libcxx-commits] [PATCH] D109475: [libc++] Simplify span specializations
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Sep 9 14:49:10 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.
================
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;
};
----------------
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.
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