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

Adapt replace(_if) to C++20 #5192

Merged
merged 5 commits into from
Feb 27, 2021
Merged

Conversation

gonidelis
Copy link
Contributor

@gonidelis gonidelis commented Feb 20, 2021

This adapts replace/replace_if/replace_copy/replace_copy_if to C++20.

Please don't merge yet, docs will be added soon.

@gonidelis gonidelis force-pushed the replace_adapt branch 2 times, most recently from 24035b6 to dc92a8a Compare February 23, 2021 15:15
hkaiser
hkaiser previously approved these changes Feb 24, 2021
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gonidelis
Copy link
Contributor Author

@hkaiser Should I also provide docs for the result types (e.g. replace_copy_result)? If yes, where exactly?

@hkaiser
Copy link
Member

hkaiser commented Feb 25, 2021

@hkaiser Should I also provide docs for the result types (e.g. replace_copy_result)? If yes, where exactly?

That would be nice. You can add the docs as doxygen comments close to the actual types. Feel free to steal the text from rangev3, if available there.

@gonidelis
Copy link
Contributor Author

This should be ok now, once CI gets greeny.

@hkaiser hkaiser merged commit 6f95aff into STEllAR-GROUP:master Feb 27, 2021
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.

Hey sorry for the late review, some minor comments about the docs otherwise LGTM thanks a lot!

/// (deduced).
/// \tparam T The type of the new values to replace (deduced).
///
/// \param policy The execution policy to use for the scheduling of
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

/// output iterator.
/// \tparam T The type of the old and new values (deduced).
///
/// \param policy The execution policy to use for the scheduling of
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

FwdIter2 dest, T1 const& old_value, T2 const& new_value,
Proj&& proj = Proj())
// clang-format off
template <typename ExPolicy, typename FwdIter1, typename Sent,
Copy link
Contributor

@Jedi18 Jedi18 Mar 9, 2021

Choose a reason for hiding this comment

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

I know this PR has been merged but did you add typename Sent by mistake? Since last is of type FwdIter1 for this deprecated overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jedi18 Great catch! Don't tell the boss, I will take care of it asap ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants