-
Notifications
You must be signed in to change notification settings - Fork 407
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
#4077 Follow-on cleanup: drop volatile-qualified members from Pair and Complex #5412
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please guard it with DEPRECATED_CODE_4
I expect they're probably unused. Could we go with 3, and see if anyone complains once 4.0 ships, so we can drop them in 4.1? I know we're technically late, but it may be immaterial. |
I would be fine with 4. I think Christian was leaning towards 3. @crtrott please advise. |
Following comments from @dalg24 I concluded the following
I will amend and rebase |
7276e24
to
b80f4ba
Compare
b80f4ba
to
228b281
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate them properly with KOKKOS_DEPRECATED
?
... Yes |
(!defined(KOKKOS_COMPILER_INTEL) || KOKKOS_COMPILER_INTEL >= 2021) | ||
#if defined(KOKKOS_ENABLE_DEPRECATION_WARNINGS) && !defined(__NVCC__) && \ | ||
(!defined(KOKKOS_COMPILER_INTEL) || KOKKOS_COMPILER_INTEL >= 2021) && \ | ||
!defined(KOKKOS_ENABLE_OPENACC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it interesting that it works in the NVHPC build and not in the OPENACC one.
They use nvc++ version 22.7 and 22.3 respectively.
Let's check if it works with more recent version of the compiler and bump the version if that happens to do the trick
@seyonglee @pedrovalerolara
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind I see the same issue with 22.9
We need to report this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5604 fixes the issue
Inspired by documentation work in kokkos/kokkos-core-wiki#151, whose corresponding PR will reflect this