Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

[WIP] Backport C++17 type_traits and C++20 chrono #10

Closed
wants to merge 5 commits into from

Conversation

trxcllnt
Copy link
Member

PR to apply the backports we did for C++17 and 20 features for cuDF.

Related cuDF PR: rapidsai/cudf#6275

@@ -1061,7 +1061,7 @@ private:
rep __rep_;
public:

#if !defined(_LIBCUDACXX_CXX03_LANG)
#if !defined(_LIBCUDACXX_CXX03_LANG) && !defined(__NVCC__)
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is suspect, but required in order to declare __shared__ durations in kernels with nvcc <= 10.2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain? This is counterintuitive to me, did it not understand = default as trivial when appropriate?

Anyway if anything, to enable this, this should be a version check and not just blank disable of = default for all versions of NVCC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's an example of a line that errors (with CUDA 10.2, haven't tried 11 or < 10.2):

__shared__ T temp_data[block_size];

../../../../include/cudf/detail/copy_if.cuh (108): error: initializer not allowed for __shared__ variable 

@trxcllnt trxcllnt changed the title Backport C++17 type_traits and C++20 chrono [WIP] Backport C++17 type_traits and C++20 chrono Sep 20, 2020
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

In general this looks good to me, but I think we'll need to add more tests - we haven't really done any calendar testing to the libcu++ test suite, because C++20 wasn't a consideration yet - but if we backport it, we'll need to do that.

@jrhemstad this may be a good project for you to get some practice with the testing system on ;)

@@ -1061,7 +1061,7 @@ private:
rep __rep_;
public:

#if !defined(_LIBCUDACXX_CXX03_LANG)
#if !defined(_LIBCUDACXX_CXX03_LANG) && !defined(__NVCC__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain? This is counterintuitive to me, did it not understand = default as trivial when appropriate?

Anyway if anything, to enable this, this should be a version check and not just blank disable of = default for all versions of NVCC.

@@ -2758,6 +2758,7 @@ inline constexpr year_month_weekday_last& year_month_weekday_last::operator-=(co
inline constexpr year_month_weekday_last& year_month_weekday_last::operator+=(const years& __dy) noexcept { *this = *this + __dy; return *this; }
inline constexpr year_month_weekday_last& year_month_weekday_last::operator-=(const years& __dy) noexcept { *this = *this - __dy; return *this; }

#if _LIBCUDACXX_STD_VER > 17
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's C++20-specific about hh_mm_ss?

Copy link
Member Author

@trxcllnt trxcllnt Sep 21, 2020

Choose a reason for hiding this comment

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

hh_mm_ss doesn't compile with gcc-7, possibly only on ubuntu16.04 (haven't tested gcc-7 on ubuntu18.04).

@jrhemstad
Copy link
Collaborator

Working on porting remaining <chrono> tests from libc++ to libcu++ so we can make sure these changes are tested properly.

@jrhemstad
Copy link
Collaborator

This PR is superseded by #44 and #45

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants