Allow passing a temporary std::vector to partition_space #6167
Conversation
| auto instances = | ||
| Kokkos::Experimental::partition_space(TEST_EXECSPACE(), weights); | ||
| auto instances = Kokkos::Experimental::partition_space( | ||
| TEST_EXECSPACE(), std::vector<int> /*weights*/ {1, 1}); |
There was a problem hiding this comment.
It does not make it obvious to the reader that you mean to call with a temporary.
Consider doing
static_assert(
std::is_invocablle_v<
decltype(Kokkos::Experimental::partition_space<int>),
std::vector<int>&&>,
"intended to be callable with temporary"
);There was a problem hiding this comment.
I would need overload resolution for Kokkos::Experimental::partition_space<ExecSpace, int>) since both overloads are eligible. That means that I would need to simulate the call before testing that the function is callable with the arguments I need to provide for overload resolution which seems to defeat the purpose.
I just added a comment making the intent clear.
|
Please check on the original PR if taking a non-const reference was a design choice. |
#4096 (comment) asked that question but there was no answer. |
* Allow passing a temporary std::vector to partition_space * Comment on ignoring content of weights * Add comment that we test that we can use temporaries for weights
Related to https://kokkosteam.slack.com/archives/C5BGU5NDQ/p1685215235868969. There doesn't seem to be a good reason not to allow passing a temporary
std::vectortopartition_space.Also, this overload is not mentioned at https://kokkos.github.io/kokkos-core-wiki/API/core/spaces/partition_space.html?highlight=partition_space#_CPPv4I0DpE15partition_spaceNSt6vectorI9ExecSpaceEERK9ExecSpaceDp4Args.