-
Notifications
You must be signed in to change notification settings - Fork 1.6k
<mdspan>: Implement *Mandates* clauses
#3535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<mdspan>: Implement *Mandates* clauses
#3535
Conversation
Since the standard suggests `is-extents` name, I'm not going to add extra suffix http://eel.is/c++draft/views.multidim#mdspan.layout.stride.expo-3
MattStephanson
left a comment
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.
Thanks for picking up some of the work!
Co-authored-by: Matt Stephanson <68978048+MattStephanson@users.noreply.github.com>
|
|
||
| _NODISCARD static constexpr bool _Is_index_space_size_representable() { | ||
| if constexpr (rank_dynamic() == 0 && rank() > 0) { | ||
| return _STD in_range<index_type>((_Extents * ...)); |
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.
Do we have to worry about integer overflow during this monster multiplication?
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.
The only references I could find to index_type being able to represent the index space size are in Preconditions to the various layout mapping constructors. If we're going to upgrade it to a static_assert, then I think we do need to check for overflow, because it can happen and the compiler is supposed to diagnose UB in constant evaluation contexts. We also don't want any false positives, since the user won't have any way to bypass this check.
|
Thanks! My suggestions (if you agree with them) can be addressed in followup PRs. |
See microsoft#3590 Also addresses microsoft#3535 (comment)
std::extentsnow accepts all signed and unsigned integer types (asTRANSITIONcomments said),Extentsare representable asIndexType,RenameStandard suggest name_Is_extentsto_Is_extents_vto indicate that this is a variable,is-extentswithout-vsuffix ([mdspan.layout.stride.expo]/3),mdspanclass,mdspanconstructor,extents::_Is_index_space_size_representablefunction (this name can be much better, so I'm open for suggestions),static_assertions for[mdspan.layout.left.overview]/2,[mdspan.layout.right.overview]/2, and[mdspan.layout.stride.overview]/2.e->_Ext(non-reserved identifier),OtherExtents->_OtherExtents(ditto),r->_Rank(ditto),_NODISCARD friend->_NODISCARD_FRIENDelement_typenested type,constuctible->constructible.