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

test: add forAllApiVersions helper function #4611

Closed

Conversation

arihantkothari
Copy link
Collaborator

@arihantkothari arihantkothari commented Jul 9, 2023

High Level Overview of Change

This PR introduces a new variadic template helper function forAllApiVersions that accepts callables to execute a set of functions over a range of versions, specifically from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion. This will be useful to avoid duplication of code. (the for loop)

Context of Change

#4552

Type of Change

  • New feature (non-breaking change which adds functionality)

@arihantkothari
Copy link
Collaborator Author

The ordering.txt file was changed because of the levelization patch.

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

LGTM. Leaving a few things for you to try out but not required to change them.

* for running a series of tests or operations that need to be performed on
* multiple versions of an API.
*/
template <class... Fs>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need the type anywhere inside the body so you could just use auto:

void forAllApiVersions(auto... callables)

You could also constraint it to require to be callable with a version number using a concept:

template<typename T>
concept VersionedTestCallable = requires(T a, uint32_t v) // use same type as real version for `v`
{
    { a(v) } -> std::same_as<void>;
};
// ...
void forAllApiVersions(VersionedTestCallable auto... fs)

Without any of this you still have an ok solution in my opinion.

Copy link
Collaborator Author

@arihantkothari arihantkothari Jul 18, 2023

Choose a reason for hiding this comment

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

Hi @godexsoft , sorry for the late reply - I've never practically used concepts before so I was learning about it.

template <typename T>
concept SingleVersionedTestCallable = requires(T a, unsigned int v)
{
    { a(v) } -> std::same_as<void>;
};

template <typename... T>
concept VersionedTestCallable = (... && SingleVersionedTestCallable<T>);
void
forAllApiVersions(VersionedTestCallable auto... testCallable)
//...

It looks like I'll have to do something like this. Does this look good ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, something like that. I'd think my version also works but i may be wrong - i did not check it. You can experiment on godbolt (compiler explorer) and see what works.

src/test/rpc/AccountTx_test.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@thejohnfreeman thejohnfreeman left a comment

Choose a reason for hiding this comment

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

I think I prefer the loop, perhaps a range-based for-loop over a boost::counting_range, but this is fine.

@intelliot intelliot added this to the 1.12 milestone Jul 19, 2023
@intelliot
Copy link
Collaborator

Suggested commit message:

test: add forAllApiVersions helper function (#4611)

Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: #4552

@intelliot
Copy link
Collaborator

@arihantkothari -

  1. Confirm that the commit message suggested above looks good (or offer an alternative)
  2. Confirm that this PR is ready to merge (when you confirm, someone will add the Passed label)

Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
@arihantkothari
Copy link
Collaborator Author

arihantkothari commented Jul 20, 2023

@arihantkothari -

  1. Confirm that the commit message suggested above looks good (or offer an alternative)
  2. Confirm that this PR is ready to merge (when you confirm, someone will add the Passed label)
  1. This commit message looks good to me.
  2. I can confirm that it is ready to be merged.
    Thanks :)

@intelliot intelliot changed the title For All Api Versions helper function to avoid code duplications test: add forAllApiVersions helper function Jul 20, 2023
@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 24, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
@manojsdoshi manojsdoshi mentioned this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
@manojsdoshi manojsdoshi mentioned this pull request Aug 19, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
@manojsdoshi manojsdoshi mentioned this pull request Aug 19, 2023
ximinez pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 21, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
Introduce a new variadic template helper function, `forAllApiVersions`,
that accepts callables to execute a set of functions over a range of
versions - from RPC::apiMinimumSupportedVersion to RPC::apiBetaVersion.
This avoids the duplication of code.

Context: XRPLF#4552
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
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants