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

Support using THRUST_WRAPPED_NAMESPACE #1077

Conversation

robertmaynard
Copy link
Contributor

Allows rmm to be used in the presence of THRUST_WRAPPED_NAMESPACE which changes the namespace location of all thrust types.

@robertmaynard robertmaynard added feature request New feature or request 2 - In Progress Currently a work in progress non-breaking Non-breaking change labels Aug 2, 2022
@robertmaynard robertmaynard added this to PR-WIP in v22.10 Release via automation Aug 2, 2022
@github-actions github-actions bot added CMake cpp Pertains to C++ code labels Aug 2, 2022
@harrism harrism changed the title rmm now supports being used with THRUST_WRAPPED_NAMESPACE Support using THRUST_WRAPPED_NAMESPACE Aug 2, 2022
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Couple of questions.

Comment on lines 22 to 24
#ifdef THRUST_WRAPPED_NAMESPACE
using namespace THRUST_WRAPPED_NAMESPACE;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity and tidiness, could these 3 lines just be put in a header that all RMM headers include?

Copy link
Member

Choose a reason for hiding this comment

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

How did you choose which headers to put this in? Is it needed in the ones that do not use Thrust like cuda_async_memory_resource.hpp?

If it is needed everywhere, you could put it in the base MR header (device_memory_resource.hpp) then all MR headers would include it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic only needs to be placed in headers that explicitly include thrust headers.

Given that rmm has 8 headers that include thrust, refactoring to a common header seems wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the pr to move the shared logic to thrust_namespace.h

@github-actions
Copy link

github-actions bot commented Sep 4, 2022

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@robertmaynard robertmaynard marked this pull request as ready for review September 23, 2022 13:28
@robertmaynard robertmaynard requested review from a team as code owners September 23, 2022 13:28
@robertmaynard robertmaynard removed the 2 - In Progress Currently a work in progress label Sep 23, 2022
@robertmaynard robertmaynard added 3 - Ready for review Ready for review by team and removed inactive-30d labels Sep 23, 2022
v22.10 Release automation moved this from PR-WIP to PR-Reviewer approved Sep 27, 2022
@harrism
Copy link
Member

harrism commented Sep 27, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6e0d65a into rapidsai:branch-22.10 Sep 27, 2022
v22.10 Release automation moved this from PR-Reviewer approved to Done Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team CMake cpp Pertains to C++ code feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants