<mdspan>: layout_left improvements
#3603
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -240,6 +240,15 @@ public: | |
| } | ||
| } | ||
|
|
||
| // TRANSITION, LWG ISSUE? I believe that this function should return 'index_type' | ||
| _NODISCARD constexpr index_type _Fwd_prod_of_extents(const rank_type _Idx) const noexcept { | ||
| index_type _Result = 1; | ||
| for (rank_type _Dim = 0; _Dim < _Idx; ++_Dim) { | ||
| _Result *= extent(_Dim); | ||
| } | ||
| return _Result; | ||
| } | ||
|
|
||
| _NODISCARD static _CONSTEVAL bool _Is_index_space_size_representable() { | ||
| if constexpr (rank_dynamic() == 0 && rank() > 0) { | ||
| return _STD in_range<index_type>((_Extents * ...)); | ||
|
|
@@ -314,24 +323,52 @@ public: | |
| constexpr mapping() noexcept = default; | ||
| constexpr mapping(const mapping&) noexcept = default; | ||
|
|
||
| constexpr mapping(const extents_type& _Exts_) noexcept : _Exts(_Exts_) { | ||
| // TRANSITION, CHECK [mdspan.layout.left.cons]/1 (REQUIRES '_Multiply_with_overflow_check' FROM #3561) | ||
| } | ||
|
|
||
| template <class _OtherExtents> | ||
| requires is_constructible_v<extents_type, _OtherExtents> | ||
| constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>) | ||
| mapping(const mapping<_OtherExtents>& _Other) noexcept | ||
| : _Exts(_Other.extents()) { | ||
| _STL_VERIFY(_STD in_range<index_type>(_Other.required_span_size()), | ||
| "Value of other.required_span_size() must be representable as a value of type index_type (N4944 " | ||
| "[mdspan.layout.left.cons]/4)."); | ||
| } | ||
|
|
||
| template <class _OtherExtents> | ||
| requires (extents_type::rank() <= 1) && is_constructible_v<extents_type, _OtherExtents> | ||
| constexpr explicit(!is_convertible_v<_OtherExtents, extents_type>) | ||
| mapping(const layout_right::mapping<_OtherExtents>& _Other) noexcept | ||
| : _Exts(_Other.extents()) { | ||
| _STL_VERIFY(_STD in_range<index_type>(_Other.required_span_size()), | ||
| "Value of other.required_span_size() must be representable as a value of type index_type (N4944 " | ||
| "[mdspan.layout.left.cons]/7)."); | ||
| } | ||
|
|
||
| template <class _OtherExtents> | ||
| requires is_constructible_v<extents_type, _OtherExtents> | ||
| constexpr explicit(extents_type::rank() > 0) mapping(const layout_stride::template mapping<_OtherExtents>& _Other) | ||
| : _Exts(_Other.extents()) { | ||
| if constexpr (extents_type::rank() > 0) { | ||
| const bool _Verify = [&]<size_t... _Indices>(index_sequence<_Indices...>) { | ||
| index_type _Prod = 1; | ||
| return ( | ||
| (_Other.stride(_Indices) | ||
| == (_Indices + 1 == extents_type::rank() | ||
| ? _Prod | ||
| : _STD exchange(_Prod, static_cast<index_type>(_Prod * _Exts.extent(_Indices + 1))))) | ||
| && ...); | ||
| } | ||
| (make_index_sequence<extents_type::rank()>{}); | ||
| _STL_VERIFY(_Verify, "For all r in the range [0, extents_type::rank()), other.stride(r) must be equal to " | ||
|
Contributor
There was a problem hiding this comment. Even if this were changed to There was a problem hiding this comment. Agreed, this is a case where the two parts should be wrapped in an |
||
| "extents().fwd-prod-of-extents(r) (N4944 [mdspan.layout.left.cons]/10.1)."); | ||
| } | ||
| _STL_VERIFY(_STD in_range<index_type>(_Other.required_span_size()), | ||
| "Value of other.required_span_size() must be representable as a value of type index_type (N4944 " | ||
| "[mdspan.layout.left.cons]/10.2)."); | ||
| } | ||
|
|
||
| constexpr mapping& operator=(const mapping&) noexcept = default; | ||
|
|
||
|
|
@@ -340,19 +377,14 @@ public: | |
| } | ||
|
|
||
| _NODISCARD constexpr index_type required_span_size() const noexcept { | ||
| return _Exts._Fwd_prod_of_extents(extents_type::rank()); | ||
| } | ||
|
|
||
| template <class... _IndexTypes> | ||
| requires (sizeof...(_IndexTypes) == extents_type::rank()) && (is_convertible_v<_IndexTypes, index_type> && ...) | ||
| && (is_nothrow_constructible_v<index_type, _IndexTypes> && ...) | ||
| _NODISCARD constexpr index_type operator()(_IndexTypes... _Indices) const noexcept { | ||
| return _Index_impl(make_index_sequence<extents_type::rank()>{}, static_cast<index_type>(_Indices)...); | ||
| } | ||
|
|
||
| _NODISCARD static constexpr bool is_always_unique() noexcept { | ||
|
|
@@ -367,44 +399,41 @@ public: | |
| return true; | ||
| } | ||
|
|
||
| _NODISCARD static constexpr bool is_unique() noexcept { | ||
| return true; | ||
| } | ||
|
|
||
| _NODISCARD static constexpr bool is_exhaustive() noexcept { | ||
| return true; | ||
| } | ||
|
|
||
| _NODISCARD static constexpr bool is_strided() noexcept { | ||
| return true; | ||
| } | ||
|
|
||
| _NODISCARD constexpr index_type stride(const rank_type _Idx) const noexcept | ||
| requires (extents_type::rank() > 0) | ||
| { | ||
| _STL_VERIFY(_Idx < extents_type::rank(), | ||
| "Value of i must be less than extents_type::rank() (N4944 [mdspan.layout.left.obs]/6)."); | ||
| return _Exts._Fwd_prod_of_extents(_Idx); | ||
| } | ||
|
|
||
| template <class _OtherExtents> | ||
| requires (extents_type::rank() == _OtherExtents::rank()) | ||
| _NODISCARD_FRIEND constexpr bool operator==(const mapping& _Left, const mapping<_OtherExtents>& _Right) noexcept { | ||
| return _Left._Exts == _Right.extents(); | ||
|
JMazurkiewicz marked this conversation as resolved.
|
||
| } | ||
|
|
||
| private: | ||
| extents_type _Exts{}; | ||
|
|
||
| template <class... _IndexTypes, size_t... _Seq> | ||
| _NODISCARD constexpr index_type _Index_impl(index_sequence<_Seq...>, _IndexTypes... _Indices) const noexcept { | ||
| _STL_INTERNAL_STATIC_ASSERT((same_as<_IndexTypes, index_type> && ...)); | ||
| index_type _Stride = 1; | ||
| index_type _Result = 0; | ||
| (((_Result += _Indices * _Stride), (_Stride *= _Exts.extent(_Seq))), ...); | ||
| return _Result; | ||
| } | ||
| }; | ||
|
|
@@ -559,7 +588,7 @@ public: | |
| && is_nothrow_constructible_v<index_type, const _OtherIndexType&> | ||
| #endif // ^^^ no workaround ^^^ | ||
| constexpr mapping(const extents_type& _Exts_, const span<_OtherIndexType, extents_type::rank()> _Strides_) noexcept | ||
| : _Exts(_Exts_) { | ||
| for (rank_type _Idx = 0; _Idx < extents_type::rank(); ++_Idx) { | ||
| _Strides[_Idx] = _Strides_[_Idx]; | ||
| } | ||
|
|
@@ -576,7 +605,7 @@ public: | |
| #endif // ^^^ no workaround ^^^ | ||
| constexpr mapping( | ||
| const extents_type& _Exts_, const array<_OtherIndexType, extents_type::rank()>& _Strides_) noexcept | ||
| : _Exts(_Exts_) { | ||
| for (rank_type _Idx = 0; _Idx < extents_type::rank(); ++_Idx) { | ||
| _Strides[_Idx] = _Strides_[_Idx]; | ||
| } | ||
|
|
@@ -789,7 +818,6 @@ public: | |
| template <class... _OtherIndexTypes> | ||
| requires (is_convertible_v<_OtherIndexTypes, index_type> && ...) | ||
| && (is_nothrow_constructible_v<index_type, _OtherIndexTypes> && ...) | ||
| && (sizeof...(_OtherIndexTypes) == rank() || sizeof...(_OtherIndexTypes) == rank_dynamic()) | ||
| && is_constructible_v<mapping_type, extents_type> && is_default_constructible_v<accessor_type> | ||
| constexpr explicit mdspan(data_handle_type _Ptr_, _OtherIndexTypes... _Exts) | ||
|
|
@@ -846,8 +874,8 @@ public: | |
| requires (is_convertible_v<_OtherIndexTypes, index_type> && ...) | ||
| && (is_nothrow_constructible_v<index_type, _OtherIndexTypes> && ...) | ||
| && (sizeof...(_OtherIndexTypes) == rank()) | ||
| _NODISCARD constexpr reference operator()(_OtherIndexTypes... _Indices) const { | ||
| return _Acc.access(_Ptr, static_cast<size_t>(_Map(static_cast<index_type>(_STD move(_Indices))...))); | ||
|
Contributor
There was a problem hiding this comment. This added cast isn't depicted in [mdspan.mdspan.members]/4. That seems deliberate, given the other places that do explicitly show similar casts. This is silencing a warning that the user has opted into about signed-to-unsigned conversions, so I'm not sure it's appropriate. Is there an existing policy about this situation? There also seems to be a tension in the design that There was a problem hiding this comment. Our usual policy is that we avoid emitting warnings that are produced entirely within our code, but we will emit warnings if the user has asked us to perform an operation on their behalf (i.e. involving types they've specified) that would warn if they wrote it themselves.
Contributor
Author
There was a problem hiding this comment.
The result of
I'm not sure if it is necessary. Maybe we could submit PR to https://github.com/cplusplus/draft with explicit |
||
| } | ||
|
|
||
| template <class _OtherIndexType> | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <concepts> | ||
| #include <cstddef> | ||
| #include <mdspan> | ||
| #include <type_traits> | ||
| #include <utility> | ||
|
|
||
| enum class IsNothrow : bool { no, yes }; | ||
|
|
||
| template <class Int, IsNothrow Nothrow = IsNothrow::yes> | ||
| struct ConvertibleToInt { | ||
| constexpr operator Int() const noexcept(std::to_underlying(Nothrow)) { | ||
| return Int{1}; | ||
| } | ||
| }; | ||
|
|
||
| struct NonConvertibleToAnything {}; | ||
|
|
||
| template <class T> | ||
| constexpr void check_implicit_conversion(T); // not defined | ||
|
|
||
| // clang-format off | ||
| template <class T, class... Args> | ||
| concept NotImplicitlyConstructibleFrom = | ||
| std::constructible_from<T, Args...> | ||
| && !requires(Args&&... args) { check_implicit_conversion<T>({std::forward<Args>(args)...}); }; | ||
| // clang-format on | ||
|
|
||
| template <class T> | ||
| inline constexpr bool is_extents_v = false; | ||
|
|
||
| template <class T, size_t... E> | ||
|
JMazurkiewicz marked this conversation as resolved.
|
||
| inline constexpr bool is_extents_v<std::extents<T, E...>> = true; | ||
|
|
||
| template <class Layout, class Mapping> | ||
| inline constexpr bool is_mapping_of_v = | ||
| std::is_same_v<typename Layout::template mapping<typename Mapping::extents_type>, Mapping>; | ||
|
|
||
| template <class M> | ||
| concept CheckNestedTypesOfLayoutMapping = | ||
| requires { | ||
| requires is_extents_v<typename M::extents_type>; | ||
| requires std::same_as<typename M::index_type, typename M::extents_type::index_type>; | ||
| requires std::same_as<typename M::rank_type, typename M::extents_type::rank_type>; | ||
| requires is_mapping_of_v<typename M::layout_type, M>; | ||
| }; | ||
|
|
||
| template <class M> | ||
| concept CheckMemberFunctionsOfLayoutMapping = requires(const M m) { | ||
| { m.extents() } -> std::same_as<const typename M::extents_type&>; | ||
| { m.required_span_size() } -> std::same_as<typename M::index_type>; | ||
| { m.is_unique() } -> std::same_as<bool>; | ||
| { m.is_exhaustive() } -> std::same_as<bool>; | ||
| { m.is_strided() } -> std::same_as<bool>; | ||
| }; | ||
|
|
||
| template <class M> | ||
| concept CheckStaticFunctionsOfLayoutMapping = requires(const M m) { | ||
| { M::is_always_strided() } -> std::same_as<bool>; | ||
| { M::is_always_exhaustive() } -> std::same_as<bool>; | ||
| { M::is_always_unique() } -> std::same_as<bool>; | ||
| std::bool_constant<M::is_always_strided()>::value; | ||
| std::bool_constant<M::is_always_exhaustive()>::value; | ||
| std::bool_constant<M::is_always_unique()>::value; | ||
| }; | ||
|
|
||
| // clang-format off | ||
| template <class M, class... Indices> | ||
| concept CheckCallOperatorOfLayoutMapping = | ||
| requires(const M m, Indices... i) { | ||
| { m(i...) } -> std::same_as<typename M::index_type>; | ||
| { m(i...) == m(static_cast<typename M::index_type>(i)...) } -> std::same_as<bool>; | ||
| }; | ||
| // clang-format on | ||
|
|
||
| template <class M> | ||
| concept CheckStrideMemberFunction = requires(M mapping, typename M::rank_type i) { | ||
| { mapping.stride(i) } -> std::same_as<typename M::index_type>; | ||
| }; | ||
|
|
||
| template <class M> | ||
| constexpr bool check_layout_mapping_requirements() { | ||
| static_assert(std::copyable<M>); | ||
| static_assert(std::equality_comparable<M>); | ||
| static_assert(std::is_nothrow_move_constructible_v<M>); | ||
| static_assert(std::is_nothrow_move_assignable_v<M>); | ||
| static_assert(std::is_nothrow_swappable_v<M>); | ||
| static_assert(CheckNestedTypesOfLayoutMapping<M>); | ||
| static_assert(CheckMemberFunctionsOfLayoutMapping<M>); | ||
| static_assert(CheckStaticFunctionsOfLayoutMapping<M>); | ||
|
|
||
| []<size_t... Indices>(std::index_sequence<Indices...>) { | ||
| static_assert(CheckCallOperatorOfLayoutMapping<M, decltype(Indices)...>); | ||
| } | ||
| (std::make_index_sequence<M::extents_type::rank()>{}); | ||
|
|
||
| if constexpr (requires(M m, typename M::rank_type i) { m.stride(i); }) { | ||
| static_assert(CheckStrideMemberFunction<M>); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| template <class MP, class E> | ||
| requires is_extents_v<E> | ||
| constexpr bool check_layout_mapping_policy_requirements() { | ||
| using X = typename MP::template mapping<E>; | ||
| static_assert(check_layout_mapping_requirements<X>()); | ||
| static_assert(std::same_as<typename X::layout_type, MP>); | ||
| static_assert(std::same_as<typename X::extents_type, E>); | ||
| return true; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| RUNALL_INCLUDE ..\concepts_latest_matrix.lst |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we usually check for precondition violations, at least outside of debug mode. Maybe
_STL_ASSERTwould be better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. As mentioned in a previous review, we should find a pattern for precondition checks and follow it consistently. It seemed that the pattern here was IDL=2 for iterator stuff and CDL for checks outside the iterators. (
_STL_ASSERTis directly controlled by_DEBUG. Yeah, we've developed inconsistency here.)