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.

Sign up for GitHub

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

Fix issues with pair #87

Closed
wants to merge 3 commits into from
Closed

Fix issues with pair #87

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eccb634
Use std::decay_t for make_pair types.
jrhemstad Jun 9, 2021
03d9243
Use remove_const on is_thrust_pair like.
jrhemstad Jun 9, 2021
37eca4d
Use auto return type.
jrhemstad Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions include/cuco/detail/pair.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ struct is_thrust_pair_like_impl<T,

template <typename T>
struct is_thrust_pair_like
: is_thrust_pair_like_impl<
std::remove_reference_t<decltype(thrust::raw_reference_cast(std::declval<T>()))>> {
: is_thrust_pair_like_impl<std::remove_const_t< // remove_const to WAR Thrust bug https://github.com/NVIDIA/thrust/issues/1456
std::remove_reference_t<decltype(thrust::raw_reference_cast(std::declval<T>()))>>> {
};

} // namespace detail
Expand Down Expand Up @@ -131,8 +131,10 @@ using pair_type = cuco::pair<K, V>;
* @return pair_type with first element `f` and second element `s`.
*/
template <typename F, typename S>
__host__ __device__ pair_type<F, S> make_pair(F&& f, S&& s) noexcept
__host__ __device__ auto make_pair(F&& f, S&& s) noexcept
{
return pair_type<F, S>{std::forward<F>(f), std::forward<S>(s)};
using decayed_F = std::decay_t<F>;
using decayed_S = std::decay_t<S>;
return pair_type<decayed_F, decayed_S>{std::forward<decayed_F>(f), std::forward<decayed_S>(s)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use move here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

std::forward will have the same semantics as std::move if F or S are r-value references.

You can think of std::forward as a more generic form of std::move.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be a return type mismatch with the function signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point! Used auto return type instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jrhemstad yeah, I guess my point is that since decayed_F is not a reference type, then forward<decayed_F> will always return an r-value, so it's identical to just using move in this case - is my understanding correct here? and so I was thinking that move is just shorter to use here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you could use std::forward with the original type as well

}
} // namespace cuco