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

swap is unnecessarily constrained on reference-like objects #802

Open
upsj opened this issue Mar 25, 2022 · 0 comments · May be fixed by NVIDIA/thrust#1646
Open

swap is unnecessarily constrained on reference-like objects #802

upsj opened this issue Mar 25, 2022 · 0 comments · May be fixed by NVIDIA/thrust#1646
Assignees
Labels
thrust For all items related to Thrust.

Comments

@upsj
Copy link

upsj commented Mar 25, 2022

When looking at the implementation of device_reference and tuple_of_iterator_references, I discovered that the swap support may be unnecessarily constraining (compared to _BitReference from libstdc++, which I originally took as a reference for how a reference-like type could be implemented):

using std::swap;
thrust::device_vector<int> vec(4);
int i{};
auto ref = vec[0];
auto ref2 = vec[1];
swap(vec[0], vec[1]); // 1
swap(ref, ref2);      // 1*
swap(i, vec[0]);      // 2
swap(vec[0], i);      // 3

auto it = thrust::make_zip_iterator(vec.begin(), vec.begin() + 1);
thrust::tuple<int, int> tuple;
swap(it[0], it[2]); // 4
swap(it[0], tuple); // 5
swap(tuple, it[0]); // 6

https://godbolt.org/z/4rbK3WhM3

1 does not work (but is intended to work, I guess?) because the swap specialization for device_reference unnecessarily (?) requires the input to be a mutable l-value reference instead of just a value, as 1* shows.
2 and 3 do not have associated specializations at all, same as 5 and 6.

This should probably be easy to fix: Add the 4 missing specializations and remove the reference qualifier.

EDIT: On second thought, adding 5 and 6 is probably highly non-trivial, as there seems to be no existing way to get the value type from a reference-like object? And to truly tackle 2 and 3, one would need to understand the details of tagged_reference.

@jrhemstad jrhemstad added the thrust For all items related to Thrust. label Feb 22, 2023
@jarmak-nv jarmak-nv assigned wmaxey and unassigned upsj Feb 23, 2023
@jarmak-nv jarmak-nv transferred this issue from NVIDIA/thrust Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
thrust For all items related to Thrust.
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants