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

Jump to bottom

Adapting transform to C++20 (non-compilable PR) #4855

Closed
wants to merge 16 commits into from
Closed

Adapting transform to C++20 (non-compilable PR) #4855

wants to merge 16 commits into from

Conversation

gonidelis
Copy link
Contributor

@gonidelis gonidelis commented Jul 24, 2020 •
edited

This is an early adaptation for transform. It includes:

  • adaptation for the end/begin iterators to be different in non-segmented transform
  • adaptation for the end/begin iterators to be different in segmented transform (didn't touch segmented_transform() functions though)
  • transform_projected specialization addition with util::projection_utility

To do

  • Add tests for the adaptations.

There is a compilation error when:

make tests.unit.modules.segmented_algorithms.partitioned_vector_transform_binary2 and
make tests.unit.modules.segmented_algorithms.partitioned_vector_transform_binary3

which I believe stems from the projection_identity addition but I can't figure out why.

  • Fix test compilation error for partitioned_vector_transform_binary2/3

Important

This is an early adaptation which aims to facilitate the final adaptation that will utilize CPOs.

@hkaiser hkaiser mentioned this pull request Jul 28, 2020
Closed
51 tasks
@gonidelis gonidelis force-pushed the transform_adapt branch 2 times, most recently from d7bb53e to fc6c343 Compare July 30, 2020 23:47
@gonidelis
Copy link
Contributor Author

gonidelis commented Jul 31, 2020 •
edited

I have adapted the result types for all three overloads of transform. There are lots of compilation errors that come from the way tests are implemented. The errors stem from here:

hpx/libs/algorithms/tests/unit/algorithms/transform_tests.hpp

Lines 66 to 67 in 256b42e

HPX_TEST(hpx::util::get<0>(result) == iterator(std::end(c)));
HPX_TEST(hpx::util::get<1>(result) == std::end(d));

and here:

hpx/libs/algorithms/tests/unit/algorithms/transform_tests.hpp

Line 94 in 256b42e

hpx::util::tuple<iterator, base_iterator> result = f.get();

hpx/libs/algorithms/tests/unit/algorithms/transform_tests.hpp

Line 162 in 256b42e

f.get();

as there is no get() function implemented for in_out_result or in_in_out_result types. I would like to discuss with @hkaiser on how to fix that.

  • I need to add a projection_identity specialization for transform_binary_projected.

After fixing the compilation issues on the tests, I think I should finally proceed on creating the CPO's.

hkaiser
hkaiser reviewed Aug 7, 2020
View reviewed changes
libs/algorithms/include/hpx/parallel/algorithms/transform.hpp
{
return get_function_address<
typename hpx::util::decay<F>::type>::call(f.f_);
return std::transform(first, last, dest);
Copy link
Member

Choose a reason for hiding this comment

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

Calling the standard algorithms will have the correct semantics, but exposes inconsistent exception behavior. For the other algorithms we have settled on relying our own implementations even in those cases where semantically the standard version would be sufficient.

aurianer
aurianer reviewed Aug 7, 2020
View reviewed changes
Copy link
Contributor

@aurianer aurianer left a comment

Choose a reason for hiding this comment

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

Very nice ;)

libs/algorithms/include/hpx/parallel/algorithms/transform.hpp Outdated
traits::projected<Proj, FwdIter1>>::value
)>
//clang-format on
HPX_DEPRECATED("hpx::parallel::transform is deprecated, use hpx::transform instead")
Copy link
Contributor

Choose a reason for hiding this comment

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

@hkaiser should we use the HPX_DEPRECATED_V macro here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what we use everywhere else now.

libs/algorithms/include/hpx/parallel/util/transform_loop.hpp
@@ -53,7 +53,7 @@ namespace hpx { namespace parallel { namespace util {
{
template <typename InIter1, typename InIter2, typename OutIter,
typename F>
HPX_HOST_DEVICE HPX_FORCEINLINE static hpx::util::tuple<InIter1,
HPX_HOST_DEVICE HPX_FORCEINLINE static util::in_in_out_result<InIter1,
InIter2, OutIter>
call(InIter1 first1, InIter1 last1, InIter2 first2, OutIter dest,
Copy link
Contributor

Choose a reason for hiding this comment

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

You may also want to adapt the begin/end iter template arguments for transform_loop too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Thanks!

@gonidelis gonidelis changed the title Transform adapt Adapting transform to C++20 Aug 7, 2020
@hkaiser hkaiser mentioned this pull request Aug 8, 2020
Closed
45 tasks
@gonidelis
Copy link
Contributor Author

gonidelis commented Aug 9, 2020 •
edited

This PR is closed and archieved. The transform C++20 adaptation will take place in a new PR #5051.

@gonidelis gonidelis closed this Aug 9, 2020
@gonidelis gonidelis changed the title Adapting transform to C++20 Adapting transform to C++20 (non-compilable PR) Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaiser hkaiser hkaiser left review comments

@aurianer aurianer aurianer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gonidelis @hkaiser @aurianer