-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Move CTAD for blocked_nd_range out of preview #1929
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
base: master
Are you sure you want to change the base?
Move CTAD for blocked_nd_range out of preview #1929
Conversation
Co-authored-by: Mike Voss <[email protected]>
Co-authored-by: Mike Voss <[email protected]>
vossmjp
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.
LGTM
| .. literalinclude:: ./examples/blocked_nd_range_ctad_example.cpp | ||
| :language: c++ | ||
| :start-after: /*begin_blocked_nd_range_ctad_example_1*/ | ||
| :end-before: /*end_blocked_nd_range_ctad_example_1*/ |
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.
What should we do with the usage examples? Is there a place in the Developer Guide to use those, or should we just delete the file?
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.
For this particular example, I guess it can be removed since it contains basic CTAD scenarios and small part of examples from the specification and the RFC.
| blocked_nd_range range({T{0}, T{100}}, {T{0}, T{100}, T{5}}, {T{0}, T{100}}, {T{0}, T{100}, T{5}}); | ||
| static_assert(std::is_same_v<decltype(range), blocked_nd_range<T, 4>>); |
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 would add a check on the grainsizes of the resulting subranges. For example, this four-dimensional range seems to be a good candidate for this as it includes subranges with different grainsizes.
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 this test case is a right place for any checks like this since setting a correct grainsize is a work for constructor, not a deduction guide.
A deduction guide should only deduce the correct type of the range and the correctness of parameters should be checked in a test for constructors (which do not use deduction guides).
By the way, I have checked such a test case for blocked_nd_range and found out that it do not contain any checks for most cases:) Updated the test case in a separate PR #1933.
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Description
Add a comprehensive description of proposed changes
Fixes #1898
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@to send notificationsOther information