-
Notifications
You must be signed in to change notification settings - Fork 758
Add transform_output_iterator and transform_input_output_iterator default constructors #1805
Add transform_output_iterator and transform_input_output_iterator default constructors #1805
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tracking the bug down!
I have some minor comment about defaulting instead of defining but besides that it looks good
run tests |
I would recommend undoing the formatting if only to make it easier to create a patch for this change. |
I believe that is fine, one can ignore whitespace changes in the github UI so the number of changes to review is rather small |
Something is strange here
|
Yeah it seesm that tbb is cranky:
|
I have no idea how to investigate TBB failures. Where do I start? How do I build with TBB locally? |
I removed the clang-format formatting changes so now the changes are only the functional ones. I still don't know how to solve the TBB failures -- I think my new test may have exposed an existing problem with the TBB backend... |
OK I split the new test into its own source file and disabled it on TBB for now. I filed NVIDIA/cccl#821 to report the TBB backend bug. |
run tests |
Cool, we're passing now. |
@wmaxey AFAIK we already have the 2.0.X branch, so I believe we should be able to merge this to main? |
Adds fix from NVIDIA/thrust#1805 to libcudf's `thrust.patch` Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Mark Harris (https://github.com/harrism) - Vyas Ramasubramani (https://github.com/vyasr) URL: #11900
Backports NVIDIA/thrust#1805 to thrust 1.17.2 since it looks to be slated for thrust 2.1 . Adding the patch to ensure that all RAPIDS projects won't be hit by this issue Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #291
@senior-zero can we merge this? |
It seems that I've fixed the initial issue with reduce by key here. In any way, having a defaulted default constructor wouldn't hurt. As I understand, the defaulted default constructors will be deleted if subobjects (output iterator and invocable) are not default constructible. |
…form_output_iterator-default-ctor Add transform_output_iterator and transform_input_output_iterator default constructors fb758b8
…-transform_output_iterator-default-ctor Add transform_output_iterator and transform_input_output_iterator default constructors fb758b8
…-transform_output_iterator-default-ctor Add transform_output_iterator and transform_input_output_iterator default constructors fb758b8
Fixes #1804
This PR adds default constructors to
transform_output_iterator
andtransform_input_output_iterator
. It also adds a unit test oftransform_output_iterator
withreduce_by_key
which fails to compile before this change. With this change it compiles and runs correctly.All other Thrust fancy iterators have default constructors, so I think this change improves consistency.
Sorry about all the formatting changes -- Thrust includes a
.clang-format
, but apparently doesn't use it to format files, so when I saved these files, clang-format reformatted them (I have format-on-save enabled). If this is a problem, I can resubmit without clang-formatting. To review just the functional changes, look forTestTransformOutputIteratorReduceByKey
, and in the headers look for= default