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

Deprecate Experimental::MasterLock #4094

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 14, 2021

(Re-open #3217)

The class template is only specialized for the OpenMP backend.
No one seems to know anything about it.
It is apparently not used anywhere.

@dalg24 dalg24 changed the base branch from master to develop June 14, 2021 15:22
The class template is only specialized for the OpenMP backend.
No one seems to know anything about it.
It is apparently not used anywhere.
@dalg24 dalg24 force-pushed the remove_experimental_master_lock branch from fdb38dd to 4f8cbf5 Compare June 14, 2021 15:45
core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -53,7 +53,9 @@
#include <impl/Kokkos_Error.hpp>
#include <impl/Kokkos_Utilities.hpp>

#ifdef KOKKOS_ENABLE_DEPRECATED_CODE_3
Copy link
Contributor

Choose a reason for hiding this comment

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

So far, we have used KOKKOS_ENABLE_DEPRECATED_CODE to print deprecations warnings, not to remove deprecated code completely (except for KOKKOS_CONSTEXPR_14 constexpr in #4063).
I would want to take this opportunity to discuss/clarify what the macro is supposed to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deprecation warnings did not work. See #3217 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I just tried to reproduce in https://godbolt.org/z/xq86aMajc and the deprecations seem to work fine for gcc, clang, icc and msvc. Would you mind reminding me what the specific issue is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to work if you specify the C++ version flag correctly, though: https://godbolt.org/z/KP14aaneW

Copy link
Member

Choose a reason for hiding this comment

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

generally KOKKOS_ENABLE_DEPRECATED_CODE should be used to remove deprecated code (if it is not defined the code is removed). Hence the name. We probably should have another option for removing warnings if required: KOKKOS_ENABLE_DEPRECATION_WARNINGS.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. In that case, I'm fine with this pull request. I'm happy to rework the current behavior of the flags in a follow-up pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW if you mark the forward declaration as deprecated you will get the warnings in your specialization with GCC (regardless of the version). I missed you had omitted it.

Copy link
Contributor

Choose a reason for hiding this comment

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

...but other compilers need the specialization to be deprecated: https://godbolt.org/z/WPz9TzMPc

Copy link
Member Author

Choose a reason for hiding this comment

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

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

4 participants