Add detection idiom from the C++ standard library extension version 2 #3980
Conversation
| #include <type_traits> | ||
|
|
||
| // NOTE This header implements the detection idiom from Version 2 of the C++ | ||
| // Extensions for Library Fundamentals, ISO/IEC TS 19568:2017 |
There was a problem hiding this comment.
Is the plan to remove this file in the future once it's part of std?
There was a problem hiding this comment.
I think so but we don't know for sure that it will actually be added to the standard library.
| // I deliberately omitted detected_or which does not fit well with the rest | ||
| // of the specification. In my opinion, it should be removed from the TS. |
There was a problem hiding this comment.
I think there are multiple occasions where we want to define a type alias if it exists and a custom default fallback type otherwise. I would just add it (in particular if you still provide detected_or_t).
There was a problem hiding this comment.
I think there are multiple occasions where we want to define a type alias if it exists and a custom default fallback type otherwise.
This is what detected_or_t provide
There was a problem hiding this comment.
Yes, I know. The *_t are normally just abbreviations for typename *::type and that's what I was referring to in the last sentence. Anyway, I don't feel strongly about it.
| using is_detected_convertible = | ||
| std::is_convertible<detected_t<Op, Args...>, To>; | ||
|
|
||
| #ifdef KOKKOS_ENABLE_CXX17 |
There was a problem hiding this comment.
Any reason to only provide this conditionally apart from the STL only providing *_v abbreviations for C++17?
There was a problem hiding this comment.
No other reason that consistency but I would be ok adding them unconditionally if reviewers disagree with my choice.
There was a problem hiding this comment.
They should be inline constexpr bool, which does require C++17. If they aren't inline, I think we may need (but haven't tested) to do something to avoid separate definitions for the same global in different translation units, such as putting them in an anonymous namespace in the header.
There was a problem hiding this comment.
Okay, so I tried the following simple test from two translation units:
template<class T>
constexpr bool beta = std::is_trivially_copyable<T>::value;
std::cout << &beta<int> << '\n';
In C++14 mode, gcc10 gives me two different addresses, which clang11 gives me the same address. I think this is an ODR-violation.
Putting them in an anonymous namespace (or declaring them static) should fix the ODR-violation.
| // NOTE This header implements the detection idiom from Version 2 of the C++ | ||
| // Extensions for Library Fundamentals, ISO/IEC TS 19568:2017 | ||
|
|
||
| // I deliberately omitted detected_or which does not fit well with the rest |
There was a problem hiding this comment.
Update the comment to also say that detected_or_t is superior to use over detected_t in all cases as the reason for removing it (it threw me off a little to see detected_or_t there until I thought about it).
There was a problem hiding this comment.
I agree that updating the comment here might help with clarity.
Take advantage of DefaultExecutionSpace::execution_space in FunctorPolicyExecutionSpace Co-Authored-By: Daniel Arndt <arndtd@ornl.gov>
| // NOTE This header implements the detection idiom from Version 2 of the C++ | ||
| // Extensions for Library Fundamentals, ISO/IEC TS 19568:2017 | ||
|
|
||
| // I deliberately omitted detected_or which does not fit well with the rest |
There was a problem hiding this comment.
I agree that updating the comment here might help with clarity.
| void operator=(nonesuch const&) = delete; | ||
| }; | ||
|
|
||
| template <template <class...> class Op, class... Args> |
There was a problem hiding this comment.
I don't know how Kokkos documentation works.. but shouldn't we have some doxygen here to explain what these things do?
There was a problem hiding this comment.
We don't use doxygen but rather rely on the Wiki for user-facing documentation.
|
You wanted to remove the last commit, isn't it? |
I was waiting on feedback from @crtrott but I will go ahead and revert |
…eduction" This reverts commit 2a650c8.
Following up on #3863 (comment)
The goal of this PR is only to add support and replace in a couple places to demonstrate usage. We will incrementally adopt and replace in the rest of Kokkos in upcoming PRs. Fixing it all up at once would be a daunting task.