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

Add remove_cvref type trait #3340

Merged
merged 3 commits into from
Sep 8, 2020
Merged

Conversation

dhollman
Copy link

@dhollman dhollman commented Sep 3, 2020

The remove_cvref type trait is added in C++20. It is useful for instance when using SFINAE on a function that take a forwarding reference. Until now we were typically using std::decay instead.

dalg24
dalg24 previously requested changes Sep 3, 2020
Comment on lines 429 to 430
struct remove_cvref : identity<typename std::remove_cv<
typename std::remove_reference<T>::type>::type> {};
Copy link
Member

Choose a reason for hiding this comment

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

Why would you use identity here?

Suggested change
struct remove_cvref : identity<typename std::remove_cv<
typename std::remove_reference<T>::type>::type> {};
struct remove_cvref : std::remove_cv_t<std::remove_reference_t<T>>::type> {};

Also please comment this is added in C++20

Copy link
Member

Choose a reason for hiding this comment

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

does clang 11 have this in C++20 mode already?

Copy link
Member

Choose a reason for hiding this comment

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

If it supports C++20 it has it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion from @dalg24 is cleaner, as we are deriving from another type trait, which already gives us the embedded type member. identity makes more sense when we don't have that, because it is more declarative in that it allows the body of the type trait struct to be empty.

It is also cleaner because of less use of typename. Many of the type traits from vendors aren't using the _t versions because those didn't appear until C++14 and they have to be backwards compatible to C++11.

Separate issue: given that C++20 settled on the name type_identity for this, we should change ours too.

@dalg24 dalg24 changed the title Graph prototype stage 3: remove_cvref Add remove_cvref type trait Sep 5, 2020
@dalg24
Copy link
Member

dalg24 commented Sep 5, 2020

NOTE to reviewer: I dropped the changes from #3339, rebased onto develop, and applied the suggestion I made since nobody provided a rational for using identity_t.
I am also proposing we just pull it from the standard library when C++20 is used.
I am unsure what is going on with clang-format.

@dalg24 dalg24 dismissed their stale review September 5, 2020 13:53

I went ahead and applied the changes I had suggested

@crtrott
Copy link
Member

crtrott commented Sep 8, 2020

The clang-format thing is interesting it looks like we have just that tiny small difference regarding closing of nested template parameters. The thing is: it seems that if the file wasn't changed in the first place it doesn't care either way, but if it applies any other change it tries to change the >> thing. Really weird.

@dalg24 dalg24 merged commit d3e94f8 into kokkos:develop Sep 8, 2020
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