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.

Sign up for GitHub

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

Jump to bottom

C++17 cleanup: traits replace std::enable_if<>::type with std::enable_if_t #10343

Merged
merged 6 commits into from
Feb 26, 2022
Merged

C++17 cleanup: traits replace std::enable_if<>::type with std::enable_if_t #10343

merged 6 commits into from
Feb 26, 2022

Conversation

karthikeyann
Copy link
Contributor

No description provided.

@karthikeyann karthikeyann added 2 - In Progress Currently a work in progress code quality libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 22, 2022
@karthikeyann karthikeyann marked this pull request as ready for review February 22, 2022 20:14
@karthikeyann karthikeyann requested a review from a team as a code owner February 22, 2022 20:14
bdice
bdice approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

👍

@karthikeyann karthikeyann requested review from a team and ttnghia February 22, 2022 20:28
@bdice
Copy link
Contributor

bdice commented Feb 22, 2022 •
edited
Loading

We have a macro CUDF_ENABLE_IF as well. I’m not sure if we should try to standardize that (or remove the macro).

@karthikeyann
Copy link
Contributor Author

We have a macro CUDF_ENABLE_IF as well. I’m not sure if we should try to standardize that (or remove the macro).

Likely most of its usage will be replaced with if constexpr, std::invocable and expression SFINAE. If all of them are removed, that macro can be removed.

jrhemstad
jrhemstad reviewed Feb 22, 2022
View reviewed changes
cpp/include/cudf/utilities/span.hpp Outdated
Comment on lines 162 to 164
typename std::enable_if_t<(Extent == OtherExtent || Extent == dynamic_extent) &&
std::is_convertible_v<OtherT (*)[], T (*)[]>,
void>* = nullptr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well go all the way. Or, can just use CUDF_ENABLE_IF macro.

Suggested change
typename std::enable_if_t<(Extent == OtherExtent || Extent == dynamic_extent) &&
std::is_convertible_v<OtherT (*)[], T (*)[]>,
void>* = nullptr>
std::enable_if_t<(Extent == OtherExtent || Extent == dynamic_extent) &&
std::is_convertible_v<OtherT (*)[], T (*)[]>>* = nullptr>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@codecov
Copy link

codecov bot commented Feb 22, 2022 •
edited
Loading

Codecov Report

Merging #10343 (1470a0b) into branch-22.04 (a7d88cd) will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10343      +/-   ##
================================================
+ Coverage         10.42%   10.58%   +0.15%     
================================================
  Files               119      125       +6     
  Lines             20603    21058     +455     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18830     +375
Impacted Files Coverage Δ
...ython/custreamz/custreamz/tests/test_dataframes.py 99.39% <0.00%> (-0.01%) ⬇️
python/cudf/cudf/errors.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/ops.py 0.00% <0.00%> (ø)
python/cudf/cudf/datasets.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/parquet.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e33453...1470a0b. Read the comment docs.

@karthikeyann
Copy link
Contributor Author

rerun tests

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4c9ef51 into rapidsai:branch-22.04 Feb 26, 2022
@karthikeyann karthikeyann added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress labels Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@ttnghia ttnghia ttnghia approved these changes

@bdice bdice bdice approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

@codereport codereport Awaiting requested review from codereport codereport is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@karthikeyann @bdice @robertmaynard @ttnghia @jrhemstad @davidwendt