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

Partially revert 1849 (refactoring changes) #1851

Closed

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented Sep 17, 2024

This PR reverts unnecessary refactoring changes done in #1849:

  • There are other occupancies of union declared as a one-liner in the library. Let's keep it consistent.

Signed-off-by: Dmitriy Sobolev <[email protected]>
lslusarczyk
lslusarczyk previously approved these changes Sep 17, 2024
__storage() {}
} __values;
__range,
([=](sycl::nd_item<1> __it)[[_ONEDPL_SYCL_REQD_SUB_GROUP_SIZE_IF_SUPPORTED(__req_sub_group_size)]] {
Copy link
Contributor

Choose a reason for hiding this comment

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

My impression was that term req_sub_group_size is already included in "_ONEDPL_SYCL_REQD_SUB_GROUP_SIZE_IF_SUPPORTED" name, therefore redundant.

But of course [[maybe-unused]] also should work, so I'm OK with the change.

Copy link
Contributor Author

@dmitriy-sobolev dmitriy-sobolev Sep 17, 2024

Choose a reason for hiding this comment

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

Yep, you are right. I also do not have strong opinion here. Let's then keep 16 as a literal if it is already included into _ONEDPL_SYCL_REQD_SUB_GROUP_SIZE_IF_SUPPORTED

Signed-off-by: Dmitriy Sobolev <[email protected]>
@dmitriy-sobolev
Copy link
Contributor Author

Closing as not relevant (the change with union was aligned with clang-format representation, let's leave it as is).

@dmitriy-sobolev dmitriy-sobolev deleted the dev/dmitriy-sobolev/partially-revert-1849 branch October 23, 2024 08:00
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.

2 participants