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 cppcoreguidelines-* to clang-tidy #10174

Closed

Conversation

codereport
Copy link
Contributor

This is a follow up PR to: #9860

It should ideally be merged after: #10064

@codereport codereport added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 31, 2022
@codereport codereport self-assigned this Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.04@83ec0af). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.04   #10174   +/-   ##
===============================================
  Coverage                ?   10.48%           
===============================================
  Files                   ?      122           
  Lines                   ?    20496           
  Branches                ?        0           
===============================================
  Hits                    ?     2148           
  Misses                  ?    18348           
  Partials                ?        0           

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 83ec0af...6292904. Read the comment docs.

@@ -223,7 +223,7 @@ std::unique_ptr<column> scatter(
auto const num_rows = target.size();
if (num_rows == 0) { return cudf::empty_like(target); }

auto lv = static_cast<list_scalar const*>(&slr);
auto lv = dynamic_cast<list_scalar const*>(&slr);
Copy link
Contributor

@bdice bdice Feb 2, 2022

Choose a reason for hiding this comment

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

We aren't testing the results of this dynamic cast, so we should use references instead of pointers.

C.148: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-ptr-cast

A failure to find the required class will cause dynamic_cast to return a null value, and de-referencing a null-valued pointer will lead to undefined behavior. Therefore the result of the dynamic_cast should always be treated as if it might contain a null value, and tested.

Instead, dynamic_cast to a reference type as recommended by C.147: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c147-use-dynamic_cast-to-a-reference-type-when-failure-to-find-the-required-class-is-considered-an-error

Suggested change
auto lv = dynamic_cast<list_scalar const*>(&slr);
auto lv = dynamic_cast<list_scalar const&>(slr);

(and then change semantics below to reference . instead of pointer ->)

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currently a work in progress 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.

2 participants