-
Notifications
You must be signed in to change notification settings - Fork 333
Description
While reviewing #5839, we discovered an issue with iterator_facade_category.
Concretely, an iterator like:
auto keys_in = thrust::make_transform_iterator(cuda::make_counting_iterator(0ULL), index_to_value_t<type>{});(see unique_by_key unit test) advertises an iterator category of std::input_iterator_tag and thus it's iterator system is cpp, and not any_system_tag. The counting iterator starts out with any_system_tag, so wrapping it in a transform iterator loses the any system tag. This is bad, because it leads to wrong systems being selected or compilation errors, which show up in #5839.
The root problem is how the iterator facade in transform iterator forms its iterator category:
template <typename CategoryOrSystem, typename CategoryOrTraversal, typename ValueParam, typename Reference>
struct iterator_facade_category
{
using type = typename ::cuda::std::_If<
is_iterator_category<CategoryOrTraversal>,
::cuda::std::type_identity<CategoryOrTraversal>,
iterator_facade_category_impl<CategoryOrSystem, CategoryOrTraversal, ValueParam, Reference>>::type;
};Transform iterator effectively instantiates this as
iterator_facade_category<thrust::any_system_tag, std::input_iterator_tag, int, int>but because std::input_iterator_tag (CategoryOrTraversal) satisfies is_iterator_category, it is just passed through and becomes the category of the transform iterator, instead of iterator_category_with_system_and_traversal<...> containing the correct information. And iterator_system<std::input_iterator_tag> is cpp later, leading to errors.
I think we should fix is_iterator_category to only be true for instantiations of iterator_category_with_system_and_traversal, but we definitely need extensive testing, since this is at the core of Thrust's iterator model.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status