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

Only declaration required as make_wrapped_policy template params #1605

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented May 28, 2024

In this PR we doing the next things:

  1. Only declaration required as make_wrapped_policy template param
  2. For aligning we declare these things always as struct.

The goal - to align these structures: their body isn't required.
And we have a lot of the same structures without body.
For example,

template <typename Name>
class ExclusiveScan1;
template <typename Name>
class ExclusiveScan2;

and so far and so on.

@SergeyKopienko SergeyKopienko requested a review from rarutyun May 28, 2024 10:59
@SergeyKopienko SergeyKopienko changed the title Only declaration required as make_wrapped_policy template param Only declaration required as make_wrapped_policy template params May 28, 2024
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/only_declaration_required_to_make_wrapped_policy branch from 1f7aa6b to 997711c Compare June 10, 2024 14:46
@SergeyKopienko SergeyKopienko added this to the 2022.7.0 milestone Jun 10, 2024
@SergeyKopienko
Copy link
Contributor Author

@rarutyun, how do you think, does it make sense to apply this PR ?

…classes for make_wrapped_policy template parameter as structures

Signed-off-by: Sergey Kopienko <[email protected]>
…re all classes for make_wrapped_policy template parameter as structures

Signed-off-by: Sergey Kopienko <[email protected]>
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/only_declaration_required_to_make_wrapped_policy branch from 997711c to b87f1c5 Compare June 28, 2024 14:18
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 don't believe there is any downside to making this change to be declaration only, since these types are merely used as indicator types to make unique and identifiable policy names. However, I am not 100% sure that making the change to declaration only is without negative consequences in all compilers / environments.

I don't see much benefit to the change, as it only removes a handful of braces / lines. I suppose it may prevent someone from doing something with the type for some other purpose beyond how it is currently used as a way to make a unique policy name.

Alignment is the most interesting part to me, but I think the most important part of an alignment would be the name, because the name is effectively the only purpose of these types. I've pointed out a few outliers and places we can improve.

I don't see a benefit to aligning class vs struct, but I don't object to it either.

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.

After offline discussion, I realize I've missed that we have many such instances of declaration only objects used in this fashion throughout oneDPL (I think I've written some).
With this in mind, I think this is safe to do. I wont insist on naming changes, because that would make this PR much larger than its original intentions, but I do think it could be a benefit.

I see no problem with this PR merging.

Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

LGTM

@SergeyKopienko SergeyKopienko merged commit 3e214c2 into main Jul 3, 2024
20 checks passed
@SergeyKopienko SergeyKopienko deleted the dev/skopienko/only_declaration_required_to_make_wrapped_policy branch July 3, 2024 13:48
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.

3 participants