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

Remove include <ranges> #4788

Merged
merged 3 commits into from Oct 30, 2023
Merged

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Oct 25, 2023

High Level Overview of Change

Remove dependency on <ranges> header, since it is not implemented by all compilers which we want to support. Resolves #4787

Context of Change

We only use ranges for std::ranges::begin and std::ranges::end, and for a matching concept. This can be trivially replaced with std::begin , std::end and an equivalent, trivial to write, concept.

The code change in question only affects unit tests.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@Bronek Bronek marked this pull request as ready for review October 25, 2023 18:15
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

Looks great. Works for me using Xcode 13.4.1.

@scottschurr
Copy link
Collaborator

Having slept on this, the current solution works for me. However, I'm having some pause about introducing a custom concept that is only used in one place. We could remove the concept and use a requires expression instead, like this:

template <typename Input>
auto
make_vector(Input const& input)
    requires requires(Input& v) {
                 std::begin(v);
                 std::end(v);
             }
{
    return std::vector(std::begin(input), std::end(input));
}

That would remove the need for a custom concept. Just a thought.

@Bronek
Copy link
Collaborator Author

Bronek commented Oct 26, 2023

Having slept on this, the current solution works for me. However, I'm having some pause about introducing a custom concept that is only used in one place. We could remove the concept and use a requires expression instead, like this:

template <typename Input>
auto
make_vector(Input const& input)
    requires requires(Input& v) {
                 std::begin(v);
                 std::end(v);
             }
{
    return std::vector(std::begin(input), std::end(input));
}

That would remove the need for a custom concept. Just a thought.

It's generic concept, and so is this function. I consider requires requires a little ugly - they are good to describe contract that a given function needs, but if something is generic enough I prefer to give it a name. Of course that raises a question of location of such a utility - the location is not generic at all here. But IMO it's better to have a named concept and plan moving it elsewhere in the future (how about XRPLF/toolset repo ?) rather than something that pretends to be very specific when actually it is not.

@seelabs
Copy link
Collaborator

seelabs commented Oct 30, 2023

I'm approving this, but in my opinion the concept doesn't add much. I probably prefer the code without the concept. The concept isn't used to help with an overload, and the expected interface of Input is trivially clear from code.

@Bronek
Copy link
Collaborator Author

Bronek commented Oct 30, 2023

I'm approving this, but in my opinion the concept doesn't add much. I probably prefer the code without the concept. The concept isn't used to help with an overload, and the expected interface of Input is trivially clear from code.

OK, I think appropriate compromise is to put a comment why this code is the way it is and why we need it temporarily i.e. because we do not yet have <ranges> on all supported compilers

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍

@Bronek
Copy link
Collaborator Author

Bronek commented Oct 30, 2023

@intelliot this is ready to merge

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 30, 2023
@intelliot intelliot merged commit ac02e56 into XRPLF:develop Oct 30, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on <ranges>
4 participants