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

Add cppcoreguidelines-* to clang-tidy #10174

Closed
wants to merge 1 commit into from
Closed

Add cppcoreguidelines-* to clang-tidy #10174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ Checks:
'modernize-*,
-modernize-use-equals-default,
-modernize-concat-nested-namespaces,
-modernize-use-trailing-return-type'

-modernize-use-trailing-return-type,
cppcoreguidelines-*,
-cppcoreguidelines-init-variables,
-cppcoreguidelines-pro-type-member-init'

# -modernize-use-equals-default # auto-fix is broken (doesn't insert =default correctly)
# -modernize-concat-nested-namespaces # auto-fix is broken (can delete code)
# -modernize-use-trailing-return-type # just a preference
Expand Down
2 changes: 1 addition & 1 deletion cpp/include/cudf/lists/detail/scatter.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 ->)

bool slr_valid = slr.is_valid(stream);
rmm::device_buffer null_mask =
slr_valid ? cudf::detail::create_null_mask(1, mask_state::UNALLOCATED, stream, mr)
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/copying/copy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ template <>
struct scalar_empty_like_functor_impl<cudf::list_view> {
std::unique_ptr<column> operator()(scalar const& input)
{
auto ls = static_cast<list_scalar const*>(&input);
auto ls = dynamic_cast<list_scalar const*>(&input);

// TODO: add a manual constructor for lists_column_view.
column_view offsets{cudf::data_type{cudf::type_id::INT32}, 0, nullptr};
Expand All @@ -79,7 +79,7 @@ template <>
struct scalar_empty_like_functor_impl<cudf::struct_view> {
std::unique_ptr<column> operator()(scalar const& input)
{
auto ss = static_cast<struct_scalar const*>(&input);
auto ss = dynamic_cast<struct_scalar const*>(&input);

// TODO: add a manual constructor for structs_column_view
// TODO: add cudf::get_element() support for structs
Expand Down
10 changes: 5 additions & 5 deletions cpp/src/interop/from_arrow.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ data_type arrow_to_cudf_type(arrow::DataType const& arrow_type)
case arrow::Type::DOUBLE: return data_type(type_id::FLOAT64);
case arrow::Type::DATE32: return data_type(type_id::TIMESTAMP_DAYS);
case arrow::Type::TIMESTAMP: {
auto type = static_cast<arrow::TimestampType const*>(&arrow_type);
auto type = dynamic_cast<arrow::TimestampType const*>(&arrow_type);
switch (type->unit()) {
case arrow::TimeUnit::type::SECOND: return data_type(type_id::TIMESTAMP_SECONDS);
case arrow::TimeUnit::type::MILLI: return data_type(type_id::TIMESTAMP_MILLISECONDS);
Expand All @@ -65,7 +65,7 @@ data_type arrow_to_cudf_type(arrow::DataType const& arrow_type)
}
}
case arrow::Type::DURATION: {
auto type = static_cast<arrow::DurationType const*>(&arrow_type);
auto type = dynamic_cast<arrow::DurationType const*>(&arrow_type);
switch (type->unit()) {
case arrow::TimeUnit::type::SECOND: return data_type(type_id::DURATION_SECONDS);
case arrow::TimeUnit::type::MILLI: return data_type(type_id::DURATION_MILLISECONDS);
Expand All @@ -78,7 +78,7 @@ data_type arrow_to_cudf_type(arrow::DataType const& arrow_type)
case arrow::Type::DICTIONARY: return data_type(type_id::DICTIONARY32);
case arrow::Type::LIST: return data_type(type_id::LIST);
case arrow::Type::DECIMAL: {
auto const type = static_cast<arrow::Decimal128Type const*>(&arrow_type);
auto const type = dynamic_cast<arrow::Decimal128Type const*>(&arrow_type);
return data_type{type_id::DECIMAL128, -type->scale()};
}
case arrow::Type::STRUCT: return data_type(type_id::STRUCT);
Expand Down Expand Up @@ -298,7 +298,7 @@ std::unique_ptr<column> dispatch_to_cudf_column::operator()<cudf::dictionary32>(
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
auto dict_array = static_cast<arrow::DictionaryArray const*>(&array);
auto dict_array = dynamic_cast<arrow::DictionaryArray const*>(&array);
auto dict_type = arrow_to_cudf_type(*(dict_array->dictionary()->type()));
auto keys_column = get_column(*(dict_array->dictionary()), dict_type, true, stream, mr);
auto ind_type = arrow_to_cudf_type(*(dict_array->indices()->type()));
Expand Down Expand Up @@ -328,7 +328,7 @@ std::unique_ptr<column> dispatch_to_cudf_column::operator()<cudf::struct_view>(
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
auto struct_array = static_cast<arrow::StructArray const*>(&array);
auto struct_array = dynamic_cast<arrow::StructArray const*>(&array);
std::vector<std::unique_ptr<column>> child_columns;
// Offsets have already been applied to child
arrow::ArrayVector array_children = struct_array->fields();
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/strings/copying/shift.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ std::unique_ptr<column> shift(strings_column_view const& input,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
auto d_fill_str = static_cast<string_scalar const&>(fill_value).value(stream);
auto d_fill_str = dynamic_cast<string_scalar const&>(fill_value).value(stream);

// output offsets column is the same size as the input
auto const input_offsets =
Expand Down