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

Add rvalue get for tuples #117

Merged
merged 6 commits into from
Oct 6, 2022
Merged

Add rvalue get for tuples #117

merged 6 commits into from
Oct 6, 2022

Conversation

MrBurmark
Copy link
Member

Here is my attempt to add support for rvalue forms of get with tuple.
This fixes #116.

@MrBurmark MrBurmark requested a review from trws October 5, 2022 16:55
Copy link
Member

@trws trws left a comment

Choose a reason for hiding this comment

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

I quite like this, but we need some tests here to ensure that the rvalue getters work as necessary. Probably not a ton to do there, but at least some basic "make_tuple(0).get()" or similar for a few different situations so we have a smoke test for this would make me a lot more comfortable it wont accidentally be broken down the line.

@MrBurmark
Copy link
Member Author

Of course, I'll add tests.

@MrBurmark
Copy link
Member Author

One thing to note about this is that it requires changes to be made in RAJA, so it will cause a break in compatibility so this change can only be made when we update everything. Do we want to add this for the immanent release or wait until a later release?

@rhornung67
Copy link
Member

I think we should get these changes in for the release.

@trws
Copy link
Member

trws commented Oct 6, 2022

Does it break any external APIs in raja? It's an ABI break because of the new overloads but if the API level at least at raja is ok I'm a lot less worried about the fallout.

@MrBurmark
Copy link
Member Author

MrBurmark commented Oct 6, 2022

It breaks stuff related to RAJA::zip_tuple. We have to get rid of the RAJA::get implementation here https://github.com/LLNL/RAJA/blob/develop/include/RAJA/util/camp_aliases.hpp#L69 which probably should never have existed in the first place. We also have to refactor the RAJA::get for zip_tuple https://github.com/LLNL/RAJA/blob/develop/include/RAJA/util/zip_tuple.hpp#L47.
I don't think anyone is using this stuff directly.

@trws
Copy link
Member

trws commented Oct 6, 2022

That seems pretty reasonable.

@trws
Copy link
Member

trws commented Oct 6, 2022

@MrBurmark, do you consider this good to go? Tests look pretty good, I would normally include some actual executable tests in with the static asserts but you have really good coverage there so we might be alright in this case.

@MrBurmark
Copy link
Member Author

I'll add some executable tests if its what you would do.

@MrBurmark
Copy link
Member Author

@trws I'm ok with this once the tests pass.

@trws
Copy link
Member

trws commented Oct 6, 2022

Looks great!

@trws trws merged commit 7e6b225 into LLNL:main Oct 6, 2022
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.

camp::get(camp::tuple) does not support rvalue references
3 participants