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

Fix -Wc++11-narrowing warnings #856

Conversation

AlexeySachkov
Copy link
Contributor

Newer versions of clang make it error by default, which affect clang-based implementations (like DPCPP).

Newer versions of `clang` make it error by default, which affect
clang-based implementations (like DPCPP).
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner January 17, 2024 13:55
@AlexeySachkov
Copy link
Contributor Author

Note for reviewers: the main change is in tests/common/type_list.h, the rest is purely mechanical and caused by the helper refactoring (function -> struct static member function).

It is possible to preserve the helper in form of a function, by doing something like:

template <typename T>
inline constexpr auto get_init_value_helper(int x) {
  return T{ static_cast<typename get_element_type<T>::type>(x) };
}

Where get_element_type is a new (or existing) type trait to get scalar element type out of vec and marray. This way the change would be less invasive. However, I do like the new name better, because with the old name one might think that function is returning some kind of helper instead of an init value. Let me know about the preference, I can rework the PR accordingly.

@bader bader added the help wanted Extra attention is needed label Jan 17, 2024
tests/common/type_list.h Outdated Show resolved Hide resolved
@@ -48,11 +48,11 @@ constexpr int default_val = 20;

template <typename T, int case_num>
constexpr sycl::specialization_id<T> spec_const(
user_def_types::get_init_value_helper<T>(default_val));
user_def_types::init_value_helper<T>::get(default_val));
Copy link
Member

Choose a reason for hiding this comment

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

What is the value of having struct instead of generic functions like

Suggested change
user_def_types::init_value_helper<T>::get(default_val));
user_def_types::get_init_value<T>(default_val));

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@keryell, I think @AlexeySachkov is seeking for discussion of this change in this comment: #856 (comment)

However, I do like the new name better, because with the old name one might think that function is returning some kind of helper instead of an init value. Let me know about the preference, I can rework the PR accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I do not like the word helper which does not help the understanding. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the value of having struct instead of generic functions like

Yes, that's a good point. We can still have it as a function, but we can also rename it along the way, I will update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored back to function, removed helper from the name in 6211bfc

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks for simplification!

@bader bader merged commit 1e7e820 into KhronosGroup:SYCL-2020 Jan 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants