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

Open

upsj wants to merge1 commit intoNVIDIA:main

base:main

Choose a base branch

fromupsj:reference_swap

Open

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

upsj wants to merge1 commit intoNVIDIA:mainfromupsj:reference_swap

Conversation

Copy link

Contributor

upsj commentedMar 25, 2022

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

ClosesNVIDIA/cccl#802

enable swap(...) for temporaries and references

191f1cc

Copy link

Collaborator

GPUtester commentedMar 25, 2022

Can one of the admins verify this patch?

ericniebler reviewed

Mar 28, 2022

View reviewed changes

Copy link

Collaborator

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 fromthrust::reference. We could simplify a lot of this by just defining some hiddenswap friend functions there.

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

testing/device_reference.cu


		// 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 testthrust::swap, no?

The existence ofthrust::swap is a bit unfortunate. If you try toswap a type that has bothstd:: andthrust:: as associated namespace, the compiler has no way to pick betweenstd::swap andthrust::swap. We could consider definingthrust::swap as a global functionobject that dispatches to (unqualified)swap in a context that has broughtstd::swap in with ausing declaration. Then a simple call tothrust::swap will do the ADL lookup internally, and the potential ambiguity betweenstd::swap andthrust::swap goes away. I think that's a change that is unlikely to break anybody.