Skip to content
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

Adding converting constructor in Kokkos::RandomAccessIterator #6929

Merged

Conversation

yasahi-hpc
Copy link
Contributor

@yasahi-hpc yasahi-hpc commented Apr 11, 2024

This PR aims at adding a missing converting constructor in Kokkos::RandomAccessIterator.
Convertibility of iterators is based on the convertibility of underlying views.

It now allows to construct a non-const iterator from a const iterator but disallows to construct a const iterator from a non-const iterator a const iterator from a non-const iterator but disallows to construct a non-const iterator from a const iterator

auto it  = KE::begin(view_from)
auto cit = KE::cbegin(view_from)
bool it2cit = std::is_constructible_v<decltype(cit), decltype(it)>; // True
bool cit2it = std::is_constructible_v<decltype(it), decltype(cit)>; // False

@yasahi-hpc yasahi-hpc self-assigned this Apr 11, 2024
@yasahi-hpc yasahi-hpc marked this pull request as draft April 11, 2024 10:07
@yasahi-hpc yasahi-hpc removed their assignment Apr 11, 2024
@yasahi-hpc yasahi-hpc marked this pull request as ready for review April 12, 2024 18:24
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments.
Looks good other than that.

ASSERT_TRUE(first_st2first_d);
ASSERT_TRUE(first_st2first_s);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that it would be good to have a test for the explicit specifier with an expression.
I just looked over the View converting constructors and they are all implicit so I don't see what we can actually do at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. I fixed accordingly.
I am less confident in the final point about a comment.
If something is wrong, please let me know

…od idea

because it let user write code that would compile with C++17 but not
with later standards.
@dalg24
Copy link
Member

dalg24 commented Apr 16, 2024

One of the CUDA build timed out (unrelated)

@dalg24 dalg24 merged commit a8115e5 into kokkos:develop Apr 16, 2024
28 of 29 checks passed
@dalg24
Copy link
Member

dalg24 commented Apr 16, 2024

@yasahi-hpc please add an entry to the changelog for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants