<mdspan>: layout_left improvements
#3603
Conversation
| _NODISCARD constexpr reference operator()(const _OtherIndexTypes... _Indices) const { | ||
| return _Acc.access(_Ptr, _Map(static_cast<index_type>(_STD move(_Indices))...)); | ||
| _NODISCARD constexpr reference operator()(_OtherIndexTypes... _Indices) const { | ||
| return _Acc.access(_Ptr, static_cast<size_t>(_Map(static_cast<index_type>(_STD move(_Indices))...))); |
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 index_type can be signed but the second parameter to access/offset is size_t. Maybe they should take a ptrdiff_t or the layout mapping should return a size_type. LWG issue needed?
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.
There was a problem hiding this comment.
There also seems to be a tension in the design that
index_typecan be signed but the second parameter toaccess/offsetissize_t. Maybe they should take aptrdiff_tor the layout mapping should return asize_type.
The result of _Map(indices...) must be non-negative and less or equal to limits<size_t>::max ([mdspan.layout.reqmts]/8), so changing parameters/return types looks like overkill to me. IMHO static_casting to size_t is enough and maybe standard should explicitly say it.
LWG issue needed?
I'm not sure if it is necessary. Maybe we could submit PR to https://github.com/cplusplus/draft with explicit static_cast (since this would not change behaviour of the code).
|
|
||
| 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()) {} | ||
| : _Exts(_Other.extents()) { | ||
| _STL_VERIFY(_STD in_range<index_type>(_Other.required_span_size()), |
There was a problem hiding this comment.
I don't think we usually check for precondition violations, at least outside of debug mode. Maybe _STL_ASSERT would be better?
There was a problem hiding this comment.
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_ASSERT is directly controlled by _DEBUG. Yeah, we've developed inconsistency here.)
| && ...); | ||
| } | ||
| (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 " |
There was a problem hiding this comment.
Even if this were changed to _STL_ASSERT, the computation above it would still be done in release mode, unless the optimizer notices that it's a dead store. As with the other instances of _STL_VERIFY, I'm not sure that's a worthwhile tradeoff.
There was a problem hiding this comment.
Agreed, this is a case where the two parts should be wrapped in an #if.
|
Happy to merge this now and followup later, or you can iterate on this PR, your pick. |
I'd like to iterate on this PR, since I've already made some changes. |
…eger; `m1`) Also, don't use `copy`. Just `cpy`. Comment: microsoft#3603 (comment)
|
LGTM. Want to merge this now? |
|
Yes, please |
|
Thanks! 😻 🚀 🎉 |
<mdspan>:layout_left::mapping's preconditions,is_(unique/exhaustive/strided)functions - they should bestatic,_Fwd_prod_of_extentstoextents,mdspan's constructor constraints,<mdspan>: Various cleanups (mostly renames) #3593 (comment).Testing:
test_mdspan_support.hppheader,layout_left: regular tests and death tests.