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

Modify is_execution_policy test to reflect change in specification #1907

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

adamfidel
Copy link
Contributor

Closes #1901.

The oneDPL specification has recently changed (uxlfoundation/oneAPI-spec#567) to move is_execution_policy and is_execution_policy_v from the oneapi::dpl::execution namespace to oneapi::dpl. This PR changes the tests to use the struct in the oneapi::dpl namespace for testing.

@danhoeflinger
Copy link
Contributor

I think that this PR is not exactly making the change suggested by the change in the specification. This PR just changes the namespace we are using and then inserts execution inline. I think we need to remove all the inline execution to comply with the "suggestion for new code". However, it probably makes sense to test both oneapi::dpl::execution and oneapi::dpl since both options are available from the specification.

@adamfidel
Copy link
Contributor Author

I think that this PR is not exactly making the change suggested by the change in the specification. This PR just changes the namespace we are using and then inserts execution inline. I think we need to remove all the inline execution to comply with the "suggestion for new code". However, it probably makes sense to test both oneapi::dpl::execution and oneapi::dpl since both options are available from the specification.

The PR is only inserting execution:: prefixes for the execution policies themselves and not for the two type traits that have moved out of that namespace (is_execution_policy and is_execution_policy_v). We still need execution:: for the policies because they are still in the execution namespace whereas the utility structs are now not.

Although I agree that it probably makes sense to test both oneapi::dpl:: and oneapi::dpl::execution:: versions of the utility structs. Let me see how I can easily do that in the same test file.

@danhoeflinger
Copy link
Contributor

The PR is only inserting execution:: prefixes for the execution policies themselves and not for the two type traits that have moved out of that namespace (is_execution_policy and is_execution_policy_v). We still need execution:: for the policies because they are still in the execution namespace whereas the utility structs are now not.

Although I agree that it probably makes sense to test both oneapi::dpl:: and oneapi::dpl::execution:: versions of the utility structs. Let me see how I can easily do that in the same test file.

Oh whoops yes my mistake. My brain short circuited and made a bad assumption. Sorry about that.

It is probably enough to have a small check of the execution namespace just that they are present, perhaps not a full copy of the test again for both namespaces.

Copy link
Contributor

@danhoeflinger danhoeflinger 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 this mostly LGTM after some small test of presence of the traits directly in the execution namespace too.

@adamfidel
Copy link
Contributor Author

I think this mostly LGTM after some small test of presence of the traits directly in the execution namespace too.

I restructured the test a bit and it now checks for the traits in both the oneapi::dpl and the oneapi::dpl::execution namespaces.

I believe this newer version simplifies the code which results in a smaller set of changes.

@danhoeflinger
Copy link
Contributor

I restructured the test a bit and it now checks for the traits in both the oneapi::dpl and the oneapi::dpl::execution namespaces.

I believe this newer version simplifies the code which results in a smaller set of changes.

Yes I like this much better, let me take a closer look but on first inspection it looks good.

danhoeflinger
danhoeflinger previously approved these changes Oct 17, 2024
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM, pending green CI

Comment on lines 48 to 55
template<typename Policy>
void assert_is_execution_policy()
{
static_assert(oneapi::dpl::is_execution_policy<Policy>::value, "wrong result for oneapi::dpl::is_execution_policy");
static_assert(oneapi::dpl::is_execution_policy_v<Policy>, "wrong result for oneapi::dpl::is_execution_policy_v");
static_assert(oneapi::dpl::execution::is_execution_policy<Policy>::value, "wrong result for oneapi::dpl::execution::is_execution_policy");
static_assert(oneapi::dpl::execution::is_execution_policy_v<Policy>, "wrong result for oneapi::dpl::execution::is_execution_policy_v");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

optional nitpick, this could be constexpr.

@adamfidel adamfidel merged commit c7cd73d into main Oct 18, 2024
22 checks passed
@adamfidel adamfidel deleted the dev/adamfidel/dpl_is_execution_policy_test branch October 18, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add testing of oneapi::dpl::is_execution_policy
2 participants