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

[oneDPL] Create a subsection for is_execution_policy type trait #567

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

akukanov
Copy link
Contributor

This PR describes oneDPL is_execution_policy trait in more detail, and also addresses #558.

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.

Generally LGTM, optional comment about the decay note.

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

Copy link
Contributor

@rarutyun rarutyun left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. I left some small comments that do look as improvements to me. But you might disagree

struct is_execution_policy { /*see below*/ };

template <class T>
constexpr bool is_execution_policy_v = oneapi::dpl::is_execution_policy<T>::value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constexpr bool is_execution_policy_v = oneapi::dpl::is_execution_policy<T>::value;
inline constexpr bool is_execution_policy_v = oneapi::dpl::is_execution_policy<T>::value;

Copy link
Contributor Author

@akukanov akukanov Oct 9, 2024

Choose a reason for hiding this comment

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

It's a template, inline should not be needed. See the standard (https://eel.is/c++draft/execution.syn):

namespace std {
  // [execpol.type], execution policy type trait
  template<class T> struct is_execution_policy;
  template<class T> constexpr bool is_execution_policy_v = is_execution_policy<T>::value;
}

Copy link
Contributor Author

@akukanov akukanov Oct 9, 2024

Choose a reason for hiding this comment

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

OK, seems inline is necessary for C++17/20. Thanks, I will get it back.

struct is_execution_policy { /*see below*/ };

template <class T>
constexpr bool is_execution_policy_v = oneapi::dpl::execution::is_execution_policy<T>::value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constexpr bool is_execution_policy_v = oneapi::dpl::execution::is_execution_policy<T>::value;
inline constexpr bool is_execution_policy_v = oneapi::dpl::execution::is_execution_policy<T>::value;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.

Comment on lines +215 to +216
to the same class template. It is unspecified in which namespace the underlying class template and its specializations
are defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
to the same class template. It is unspecified in which namespace the underlying class template and its specializations
are defined.
to the same class template. It is unspecified, which namespace the underlying class template and its specializations
are defined in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spellcheckers do not complain :), so I will keep this intact.

The ``oneapi::dpl::execution::is_execution_policy`` class originated in the oneDPL specification version 1.0,
while ``oneapi::dpl::is_execution_policy`` has been added later to better align with the C++ standard.

For writing new code, use of the type trait utilities in ``namespace oneapi::dpl`` is strongly recommended. Those
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For writing new code, use of the type trait utilities in ``namespace oneapi::dpl`` is strongly recommended. Those
For a new code, use of the type trait utilities in ``namespace oneapi::dpl`` is highly encouraged. Those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a similar change.

Comment on lines +225 to +226
``is_execution_policy<T>`` must have the characteristics of ``std::true_type`` if ``T`` is one of the above specified or
implementation-defined oneDPL execution policy types, otherwise it must have the characteristics of ``std::false_type``.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this super generic and kind of vague wording?

I specifically mean "is one of the above specified or implementation-defined oneDPL execution policy types".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compare with the C++ standard wording:

is_execution_policy<T> is a Cpp17UnaryTypeTrait with a base characteristic of true_type if T is the type of a standard or implementation-defined execution policy, otherwise false_type.

It is assumed that an implementation can define additional policy types, for which the trait also resolves to true_type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants