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

Ranges Algorithms Modernization - Return #13091

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mitaclaw
Copy link
Contributor

@mitaclaw mitaclaw commented Oct 1, 2024

In C++20, algorithms from the standard library evolved to feature new niceties and additional compile-time checks. A few of these updated algorithm functions (found in the std::ranges namespace) have new return values, and thus code beyond the expression being modernized must also be updated. I have isolated these changes so that they may be recognized and more easily understood.

@mitaclaw
Copy link
Contributor Author

mitaclaw commented Oct 1, 2024

This PR will remain a draft until I am confident there are no mistakes in my changes.

Special shout-out to the PR that inspired me to attempt this modernization: #12992

@BhaaLseN
Copy link
Member

BhaaLseN commented Oct 2, 2024

Those std::erase changes seem a lot more verbose, is that really worth it?

@mitaclaw
Copy link
Contributor Author

mitaclaw commented Oct 5, 2024

It's definitely subjective. The impression that the standard library's algorithm design gives me is that the returned subrange is the intended way to obtain the first and last iterator for erasing. It was a little unfortunate that this required a new variable in three places, but I preferred the newer design for being straightforward. I think it's worth it aesthetically and for consistency in usage, but if it's an eyesore to everyone else I can revert to obtaining the end iterator from the original container (thus making these three examples a one-liner again).

Also, I failed to notice this before (I guess I was a little sleep deprived?), but I actually fixed UB in OpenModeToAndroid with my changes. Previously, any openmode string that didn't have a 'b' character in it would erase the end iterator, which is no good.

The new return value is `std::ranges::mismatch_result`, an alias for the pair-like type `std::ranges::in_in_result`.
The new return value is `std::ranges::subrange`.
The new return value is `std::ranges::subrange`.
The new return value is `std::ranges::subrange`.
The new return value is `std::ranges::subrange`.
@mitaclaw mitaclaw force-pushed the ranges-modernization-2-returns branch from a26cca0 to d4cc0aa Compare October 5, 2024 06:12
@mitaclaw
Copy link
Contributor Author

mitaclaw commented Oct 5, 2024

Rebase with no changes; the buildbots only failed the first time because they timed out.

Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

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

If you prefer, you can also use the PairLike operator for the unique results instead of calling begin() and end():

  const auto [begin, end] = std::ranges::unique(result);
  result.erase(begin, end);

Comment on lines +65 to +66
const auto remove_result = std::ranges::remove(mode, 'b');
mode.erase(remove_result.begin(), remove_result.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't C++20's std::erase be a better fit here?

@BhaaLseN
Copy link
Member

BhaaLseN commented Oct 5, 2024

I don't have any strong feelings either way, it's just that most of the time, longer code isn't always the better code; idiomatic or not.
That being said, don't let me block you from this.

However, I was curious and tried sepalani's suggestion with std::erase. It works for the string, but I couldn't find a way to make the two vectors work with std::ranges::unique while keeping it fairly concise.

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

Successfully merging this pull request may close these issues.

3 participants