-
Notifications
You must be signed in to change notification settings - Fork 50
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
RangePolicy constructor updates from Release 4.3 #523
Conversation
Get rid of extraneous inline, const, etc. Fix parameters passed to ctors Fix return type of set_chunk_size()
// until 4.3 | ||
template<class ... Args> | ||
inline RangePolicy( const execution_space & work_space | ||
, const member_type work_begin | ||
, const member_type work_end | ||
, Args ... args); | ||
RangePolicy( const execution_space & work_space | ||
, member_type work_begin | ||
, member_type work_end | ||
, Args ... args ); | ||
|
||
// until 4.3 | ||
template<class ... Args> | ||
inline RangePolicy( const member_type work_begin | ||
, const member_type work_end | ||
, Args ... args); | ||
RangePolicy( member_type work_begin | ||
, member_type work_end | ||
, Args ... args ); | ||
|
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 even want to bother mentioning the previous interface?
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 was a little torn on this. I'll remove it. The only reason to keep it is to match compilation errors with the implementation (even though we don't guarantee any particular implementation).
ctors were templated, but that only allowed passing ChunkSize multiple times
Kokkos::RangePolicy<ARGS>(Space(), begin, end, args...) | ||
Kokkos::RangePolicy<...>(begin, end) | ||
Kokkos::RangePolicy<...>(begin, end, chunk_size) | ||
Kokkos::RangePolicy<...>(Space(), begin, end) |
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.
Kokkos::RangePolicy<...>(Space(), begin, end) | |
Kokkos::RangePolicy(exec, begin, end) |
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'm going to have both template parameters and CTAD constructors, as I don't want the usage to imply this is not a templated class.
inline RangePolicy(); | ||
RangePolicy(); | ||
|
||
RangePolicy( member_type work_begin |
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'd say index_type
I think using member type was an attempt to have genericity between exec policy but it clearly does not work with MDRangePolicy.
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.
Changed to index_type
.
|
||
Preconditions: | ||
^^^^^^^^^^^^^^ | ||
|
||
* The start index must not be greater than the end index. |
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.
You referred to the PR that check that the indices arguments are representable as index_type
.
Maybe that would be confusing because we pretend that the constructors take member_type/index_type by value when in reality they are templated.
Can we really pretend it is QOI?
(this does not need to be resolved here)
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 added a sentence under "Preconditions" (I might move it, though).
RangePolicy(const RangePolicy&) = default; | ||
RangePolicy(RangePolicy&&) = default; |
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 have gotten rid of these guys but I suppose we can do that later
This updates the existing RangePolicy constructor documentation as per kokkos/kokkos#6845 and adds a section for the CTAD constructors as per kokkos/kokkos#6850.
This addresses issue #519 and part of issue #515.