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 nested types in functional #5185

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

JBludau
Copy link
Contributor

@JBludau JBludau commented Jul 7, 2022

The functions in Kokkos_Functional.hpp mimic parts of <functional> but are device portable. See greater as example.
In the std lib the nested typenames result_type, first_argument_type, and second_argument_type are deprecated in c++17 and removed in 20. The respective functions in Kokkos should be marked deprecated when bumping to c++17

Unfortunately, it looks like using declarations can not be attributed with [[deprecated]] but typedef can.

@JBludau JBludau added Deprecate Items to be deprecated C++17 Changes that require C++17 support labels Jul 7, 2022
@JBludau JBludau changed the title Deprecate first second args in functional Deprecate nested types in functional Jul 7, 2022
@masterleinad
Copy link
Contributor

Unfortunately, it looks like using declarations can not be attributed with [[deprecated]] but typedef can.

https://godbolt.org/z/9rYv9xxa8 shows that putting [[deprecated]] last seems to work.

@JBludau JBludau force-pushed the deprecate_first_second_args_function branch from 3b71a09 to 4354fe6 Compare July 8, 2022 01:14
@JBludau
Copy link
Contributor Author

JBludau commented Jul 8, 2022

Unfortunately, it looks like using declarations can not be attributed with [[deprecated]] but typedef can.

https://godbolt.org/z/9rYv9xxa8 shows that putting [[deprecated]] last seems to work.

Interesting

@crtrott crtrott marked this pull request as ready for review July 8, 2022 18:10
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Do we want to remove it with Kokkos 4 or when using C++20 support? If the former one, we should adjust the comments and wrap it in #ifdef KOKKOS_ENABLE_DEPRECATED_CODE_3. In the latter case with #if defined(KOKKOS_ENABLE_CXX14) || defined(KOKKOS_ENABLE_CXX17).

@JBludau
Copy link
Contributor Author

JBludau commented Jul 8, 2022

Do we want to remove it with Kokkos 4 or when using C++20 support? If the former one, we should adjust the comments and wrap it in #ifdef KOKKOS_ENABLE_DEPRECATED_CODE_3. In the latter case with #if defined(KOKKOS_ENABLE_CXX14) || defined(KOKKOS_ENABLE_CXX17).

I think we should stay in line with what the std does here

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Fine with me.

@JBludau
Copy link
Contributor Author

JBludau commented Jul 8, 2022

CI failure seems unrelated
1/37 Test #1: KokkosCore_UnitTest_OpenMP ...................***Timeout 1500.69 sec

using result_type = uint32_t;
#if defined KOKKOS_ENABLE_CXX14 || defined KOKKOS_ENABLE_CXX17
using argument_type KOKKOS_DEPRECATED_WITH_COMMENT_17(
"(deprecated in C++17)(removed in C++20)") = T;
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the deprecation message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some discussion with Damien I propose to drop the message completely. Hinting towards the cause (deprecation of binary_function in the std) is kind of misleading and this indicates that compilers are not rushing to remove it.

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I would have gone for

#ifdef KOKKOS_ENABLE_DEPRECATED_CODE_3
   using first_argument_type KOKKOS_DEPRECATED  = T;
   using second_argument_type KOKKOS_DEPRECATED = T;
   using result_type KOKKOS_DEPRECATED          = bool;
#endif

I would prefer to plan for simple removal in Kokkos 4.

@JBludau JBludau force-pushed the deprecate_first_second_args_function branch from 2ae9984 to a006750 Compare July 11, 2022 12:49
@JBludau
Copy link
Contributor Author

JBludau commented Jul 11, 2022

ci failure unrelated

@dalg24 dalg24 merged commit ba28a87 into kokkos:develop Jul 11, 2022
@JBludau JBludau deleted the deprecate_first_second_args_function branch July 12, 2022 02:31
@ajpowelsnl
Copy link
Contributor

FYI - cppreference details deprecation of argument_type, first_argument_type, second_argument_type.

What will the usage be after deprecation?

@masterleinad
Copy link
Contributor

What will the usage be after deprecation?

They are not necessary anymore as explained, e.g., in https://stackoverflow.com/a/35907663, because of decltype and perfect forwarding.

@ajpowelsnl ajpowelsnl mentioned this pull request Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++17 Changes that require C++17 support Deprecate Items to be deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants