Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Enable swap(...) for temporaries and between different reference types #1646

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

upsj
Copy link
Contributor

@upsj upsj commented Mar 25, 2022

An attempt at resolving the issues surrounding swap(...) between temporary device references and between different reference(-like) types

Closes NVIDIA/cccl#802

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Collaborator

@ericniebler ericniebler left a comment

Choose a reason for hiding this comment

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

All the thrust reference-like types inherit from thrust::reference. We could simplify a lot of this by just defining some hidden swap friend functions there.

And that makes me wonder if we want to support arbitrary mixing of reference-like types (e.g., swapping a device_reference with kinds of tagged_reference). Maybe @jrhemstad could comment on that?

@@ -218,14 +219,37 @@ void TestDeviceReferenceSwap(void)
ref2 = 13;

// test thrust::swap()
thrust::swap(ref1, ref2);
swap(ref1, ref2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the point of this code is to test thrust::swap, no?

The existence of thrust::swap is a bit unfortunate. If you try to swap a type that has both std:: and thrust:: as associated namespace, the compiler has no way to pick between std::swap and thrust::swap. We could consider defining thrust::swap as a global function object that dispatches to (unqualified) swap in a context that has brought std::swap in with a using declaration. Then a simple call to thrust::swap will do the ADL lookup internally, and the potential ambiguity between std::swap and thrust::swap goes away. I think that's a change that is unlikely to break anybody.

@allisonvacanti thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, your proposed fix sounds good to me so long as nothing is specializing thrust::swap.

The THRUST_INLINE_CONSTANT macro may be needed when defining the function object, see https://github.com/NVIDIA/thrust/blob/main/thrust/detail/config/cpp_compatibility.h#L47-L74

@ericniebler ericniebler added type: enhancement New feature or request. P2: nice to have Desired, but not necessary. labels Mar 28, 2022
@ericniebler ericniebler added this to Need Review in PR Tracking Mar 28, 2022
@alliepiper alliepiper added this to the 1.17.0 milestone Apr 4, 2022
@alliepiper alliepiper modified the milestones: 1.17.0, 2.0.0 Apr 25, 2022
@alliepiper alliepiper modified the milestones: 2.0.0, 2.1.0 Jul 25, 2022
@alliepiper alliepiper moved this from Need Review to Drafts in PR Tracking Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2: nice to have Desired, but not necessary. type: enhancement New feature or request.
Projects
Development

Successfully merging this pull request may close these issues.

swap is unnecessarily constrained on reference-like objects
4 participants