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

Avoid more warnings with clang and C++20 #3719

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

masterleinad
Copy link
Contributor

Since all of the changed enums are in internal classes any ODR-usage should also be internal. Hence, I didn't provide extra definitions in namescape scope. Also, we don't have them for the corresponding HIP variables.

The changes to the comparison operators in core/src/impl/Kokkos_Atomic_View.hpp rely on the existence of implicit conversion operators.

core/src/Cuda/Kokkos_Cuda_Instance.hpp Show resolved Hide resolved
core/unit_test/TestAtomics.hpp Show resolved Hide resolved
KOKKOS_INLINE_FUNCTION
bool operator==(volatile const_value_type& val) const { return *ptr == val; }
bool operator==(volatile const AtomicDataElement& val) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

While not a comment on this change, the volatile comparisons seem sketchy.

core/src/impl/Kokkos_ViewLayoutTiled.hpp Outdated Show resolved Hide resolved
core/src/Cuda/Kokkos_Cuda_Instance.hpp Outdated Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

@ndellingwood should review. Readt from my side. More reviews/opinions welcome. Related to #3716.

Copy link
Contributor

@ndellingwood ndellingwood left a comment

Choose a reason for hiding this comment

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

lgtm @masterleinad I think intel is now PR tested via the github actions, but let me know if would you like me to do any additional testing

@ndellingwood
Copy link
Contributor

I should clarify, these changes were not made to address anything intel-related, but replacing enums with static constexpr variables in the past had caused intel compilation issues, though I don't think I've seen that since intel/15 or intel/16

@masterleinad
Copy link
Contributor Author

I should clarify, these changes were not made to address anything intel-related, but replacing enums with static constexpr variables in the past had caused intel compilation issues, though I don't think I've seen that since intel/15 or intel/16

It would probably be good to give it a full test run of the Sandia testers. The alternative to the switching to static constexpr is to have the static_casts everywhere for now as you do in #3716.

@crtrott
Copy link
Member

crtrott commented Jan 14, 2021

I just merge this and we check tomorrow if it broke anything and then we can revert. I think the likelyhood is very low. We did a similar change in some other places and it passed.

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.

None yet

5 participants