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

C++17 cleanup: traits replace ::value with _v #10319

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

karthikeyann
Copy link
Contributor

  • replace std::is_*<T>::value with std::is_*_v<T>

Please suggest any other C++17 tech debts.

@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team code quality 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 17, 2022
@karthikeyann karthikeyann requested a review from a team as a code owner February 17, 2022 14:07
@karthikeyann karthikeyann self-assigned this Feb 17, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 17, 2022
@karthikeyann
Copy link
Contributor Author

rerun tests

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🔥

@codereport codereport changed the title cpp17 cleanup: traits replace ::value with _v C++17 cleanup: traits replace ::value with _v Feb 17, 2022
@vyasr
Copy link
Contributor

vyasr commented Feb 17, 2022

@karthikeyann I believe the failure here is caused by the issue described in #10323. tl;dr merge in the latest branch-22.04 and I think style checks will pass.

@vyasr
Copy link
Contributor

vyasr commented Feb 17, 2022

Please suggest any other C++17 tech debts.

Here are some ideas off the top of my head. I'll come back and add to this list later if I think of anything, but other reviewers should feel free to edit this comment and add too. These are definitely out of scope for this PRs and could be added separately; in fact, this comment should probably be converted into a separate issue after we've compiled ideas.

  • Use CTAD to get rid of methods like std::make_pair or std::make_unique (make_unique may be a little controversial since it parallels make_shared, which still has advantages over the shared_ptr constructor).
  • Use structured bindings instead of std::tie (where possible)
  • Add more [[nodiscard]] where appropriate

More complicated changes or changes that require more case-by-case analysis:

  • Make more use of std::optional
  • Replace SFINAE with if constexpr in cases where the latter is significantly more compact
  • Use std::is_invocable to replace lots of complex SFINAE.

@ttnghia
Copy link
Contributor

ttnghia commented Feb 18, 2022

@karthikeyann You seem to miss std::enable_if 😃

@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #10319 (cbb1b8b) into branch-22.04 (a7d88cd) will increase coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10319      +/-   ##
================================================
+ Coverage         10.42%   10.63%   +0.20%     
================================================
  Files               119      122       +3     
  Lines             20603    20953     +350     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18725     +270     
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/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%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
... and 33 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 858ab83...cbb1b8b. Read the comment docs.

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c163886 into rapidsai:branch-22.04 Feb 22, 2022
@vyasr vyasr mentioned this pull request Feb 23, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants