Skip to content

Commit

Permalink
Add clang-tidy to libcudf (#9860)
Browse files Browse the repository at this point in the history
This PR is adding clang-tidy to cudf and adding the initial checks. Note more checks will be enabled in the future.

Relevant PRs:
* `rmm`: rapidsai/rmm#857
* `cuml`: rapidsai/cuml#1945

To do list:
* [x] Add `.clang-tidy` file
* [x] Add python script
* [x] Apply `modernize-` changes
* [x] Revert `cxxopts` changes
* [x] Fixed Python parquet failures
* [x] Ignore `cxxopts` file
* [x] Ignore the `build/_deps` directories

Splitting out the following into a separate PR so we can get the changes merged for 22.02 (#10064):
* ~~[ ] Disable `clang-diagnostic-errors/warnings`~~
* ~~[ ] Fix include files being skipped~~
* ~~[ ] Set up CI script~~
* ~~[ ] Clean up python script~~

Authors:
  - Conor Hoekstra (https://github.com/codereport)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Nghia Truong (https://github.com/ttnghia)
  - David Wendt (https://github.com/davidwendt)
  - Mark Harris (https://github.com/harrism)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #9860
  • Loading branch information
codereport authored Jan 20, 2022
1 parent 13429ff commit 276bcf4
Show file tree
Hide file tree
Showing 159 changed files with 1,410 additions and 1,020 deletions.
27 changes: 27 additions & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
Checks:
'modernize-*,
-modernize-use-equals-default,
-modernize-concat-nested-namespaces,
-modernize-use-trailing-return-type'

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

WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: none
CheckOptions:
- key: modernize-loop-convert.MaxCopySize
value: '16'
- key: modernize-loop-convert.MinConfidence
value: reasonable
- key: modernize-pass-by-value.IncludeStyle
value: llvm
- key: modernize-replace-auto-ptr.IncludeStyle
value: llvm
- key: modernize-use-nullptr.NullMacros
value: 'NULL'
...
4 changes: 2 additions & 2 deletions cpp/benchmarks/common/generate_benchmark_input.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,8 @@ class data_profile {

auto get_bool_probability() const { return bool_probability; }
auto get_null_frequency() const { return null_frequency; };
auto get_cardinality() const { return cardinality; };
auto get_avg_run_length() const { return avg_run_length; };
[[nodiscard]] auto get_cardinality() const { return cardinality; };
[[nodiscard]] auto get_avg_run_length() const { return avg_run_length; };

// Users should pass integral values for bounds when setting the parameters for types that have
// discrete distributions (integers, strings, lists). Otherwise the call with have no effect.
Expand Down
6 changes: 4 additions & 2 deletions cpp/benchmarks/copying/contiguous_split_benchmark.cu
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@ void BM_contiguous_split_common(benchmark::State& state,
std::vector<std::unique_ptr<cudf::column>> columns(src_cols.size());
std::transform(src_cols.begin(), src_cols.end(), columns.begin(), [](T& in) {
auto ret = in.release();
ret->null_count();
// computing the null count is not a part of the benchmark's target code path, and we want the
// property to be pre-computed so that we measure the performance of only the intended code path
[[maybe_unused]] auto const nulls = ret->null_count();
return ret;
});
cudf::table src_table(std::move(columns));
auto const src_table = cudf::table(std::move(columns));

for (auto _ : state) {
cuda_event_timer raii(state, true); // flush_l2_cache = true, stream = 0
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/copying/gather_benchmark.cu
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ template <class TypeParam, bool coalesce>
void BM_gather(benchmark::State& state)
{
const cudf::size_type source_size{(cudf::size_type)state.range(0)};
const cudf::size_type n_cols = (cudf::size_type)state.range(1);
const auto n_cols = (cudf::size_type)state.range(1);

// Every element is valid
auto data = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i; });
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/copying/scatter_benchmark.cu
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ template <class TypeParam, bool coalesce>
void BM_scatter(benchmark::State& state)
{
const cudf::size_type source_size{(cudf::size_type)state.range(0)};
const cudf::size_type n_cols = (cudf::size_type)state.range(1);
const auto n_cols = (cudf::size_type)state.range(1);

// Every element is valid
auto data = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i; });
Expand Down
13 changes: 8 additions & 5 deletions cpp/benchmarks/fixture/benchmark_fixture.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,22 @@ inline auto make_pool()
*/
class benchmark : public ::benchmark::Fixture {
public:
virtual void SetUp(const ::benchmark::State& state)
void SetUp(const ::benchmark::State& state) override
{
mr = make_pool();
rmm::mr::set_current_device_resource(mr.get()); // set default resource to pool
}

virtual void TearDown(const ::benchmark::State& state)
void TearDown(const ::benchmark::State& state) override
{
// reset default resource to the initial resource
rmm::mr::set_current_device_resource(nullptr);
mr.reset();
}

// eliminate partial override warnings (see benchmark/benchmark.h)
virtual void SetUp(::benchmark::State& st) { SetUp(const_cast<const ::benchmark::State&>(st)); }
virtual void TearDown(::benchmark::State& st)
void SetUp(::benchmark::State& st) override { SetUp(const_cast<const ::benchmark::State&>(st)); }
void TearDown(::benchmark::State& st) override
{
TearDown(const_cast<const ::benchmark::State&>(st));
}
Expand All @@ -102,7 +102,10 @@ class memory_stats_logger {

~memory_stats_logger() { rmm::mr::set_current_device_resource(existing_mr); }

size_t peak_memory_usage() const noexcept { return statistics_mr.get_bytes_counter().peak; }
[[nodiscard]] size_t peak_memory_usage() const noexcept
{
return statistics_mr.get_bytes_counter().peak;
}

private:
rmm::mr::device_memory_resource* existing_mr;
Expand Down
4 changes: 2 additions & 2 deletions cpp/benchmarks/io/csv/csv_reader_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class CsvRead : public cudf::benchmark {

void BM_csv_read_varying_input(benchmark::State& state)
{
auto const data_types = get_type_or_group(state.range(0));
io_type const source_type = static_cast<io_type>(state.range(1));
auto const data_types = get_type_or_group(state.range(0));
auto const source_type = static_cast<io_type>(state.range(1));

auto const tbl = create_random_table(data_types, num_cols, table_size_bytes{data_size});
auto const view = tbl->view();
Expand Down
4 changes: 2 additions & 2 deletions cpp/benchmarks/io/csv/csv_writer_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class CsvWrite : public cudf::benchmark {

void BM_csv_write_varying_inout(benchmark::State& state)
{
auto const data_types = get_type_or_group(state.range(0));
io_type const sink_type = static_cast<io_type>(state.range(1));
auto const data_types = get_type_or_group(state.range(0));
auto const sink_type = static_cast<io_type>(state.range(1));

auto const tbl = create_random_table(data_types, num_cols, table_size_bytes{data_size});
auto const view = tbl->view();
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/io/orc/orc_reader_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void BM_orc_read_varying_input(benchmark::State& state)
cudf::size_type const run_length = state.range(2);
cudf_io::compression_type const compression =
state.range(3) ? cudf_io::compression_type::SNAPPY : cudf_io::compression_type::NONE;
io_type const source_type = static_cast<io_type>(state.range(4));
auto const source_type = static_cast<io_type>(state.range(4));

data_profile table_data_profile;
table_data_profile.set_cardinality(cardinality);
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/io/orc/orc_writer_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void BM_orc_write_varying_inout(benchmark::State& state)
cudf::size_type const run_length = state.range(2);
cudf_io::compression_type const compression =
state.range(3) ? cudf_io::compression_type::SNAPPY : cudf_io::compression_type::NONE;
io_type const sink_type = static_cast<io_type>(state.range(4));
auto const sink_type = static_cast<io_type>(state.range(4));

data_profile table_data_profile;
table_data_profile.set_cardinality(cardinality);
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/io/parquet/parquet_reader_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void BM_parq_read_varying_input(benchmark::State& state)
cudf::size_type const run_length = state.range(2);
cudf_io::compression_type const compression =
state.range(3) ? cudf_io::compression_type::SNAPPY : cudf_io::compression_type::NONE;
io_type const source_type = static_cast<io_type>(state.range(4));
auto const source_type = static_cast<io_type>(state.range(4));

data_profile table_data_profile;
table_data_profile.set_cardinality(cardinality);
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/io/parquet/parquet_writer_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void BM_parq_write_varying_inout(benchmark::State& state)
cudf::size_type const run_length = state.range(2);
cudf_io::compression_type const compression =
state.range(3) ? cudf_io::compression_type::SNAPPY : cudf_io::compression_type::NONE;
io_type const sink_type = static_cast<io_type>(state.range(4));
auto const sink_type = static_cast<io_type>(state.range(4));

data_profile table_data_profile;
table_data_profile.set_cardinality(cardinality);
Expand Down
2 changes: 1 addition & 1 deletion cpp/benchmarks/lists/copying/scatter_lists_benchmark.cu
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void BM_lists_scatter(::benchmark::State& state)

const size_type base_size{(size_type)state.range(0)};
const size_type num_elements_per_row{(size_type)state.range(1)};
const size_type num_rows = (size_type)ceil(double(base_size) / num_elements_per_row);
const auto num_rows = (size_type)ceil(double(base_size) / num_elements_per_row);

auto source_base_col = make_fixed_width_column(
data_type{type_to_id<TypeParam>()}, base_size, mask_state::UNALLOCATED, stream, mr);
Expand Down
6 changes: 3 additions & 3 deletions cpp/benchmarks/type_dispatcher/type_dispatcher_benchmark.cu
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ void launch_kernel(cudf::mutable_table_view input, T** d_ptr, int work_per_threa
template <class TypeParam, FunctorType functor_type, DispatchingType dispatching_type>
void type_dispatcher_benchmark(::benchmark::State& state)
{
const cudf::size_type source_size = static_cast<cudf::size_type>(state.range(1));
const auto source_size = static_cast<cudf::size_type>(state.range(1));

const cudf::size_type n_cols = static_cast<cudf::size_type>(state.range(0));
const auto n_cols = static_cast<cudf::size_type>(state.range(0));

const cudf::size_type work_per_thread = static_cast<cudf::size_type>(state.range(2));
const auto work_per_thread = static_cast<cudf::size_type>(state.range(2));

auto data = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i; });

Expand Down
12 changes: 6 additions & 6 deletions cpp/include/cudf/aggregation.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ class aggregation {
Kind kind; ///< The aggregation to perform
virtual ~aggregation() = default;

virtual bool is_equal(aggregation const& other) const { return kind == other.kind; }
virtual size_t do_hash() const { return std::hash<int>{}(kind); }
virtual std::unique_ptr<aggregation> clone() const = 0;
[[nodiscard]] virtual bool is_equal(aggregation const& other) const { return kind == other.kind; }
[[nodiscard]] virtual size_t do_hash() const { return std::hash<int>{}(kind); }
[[nodiscard]] virtual std::unique_ptr<aggregation> clone() const = 0;

// override functions for compound aggregations
virtual std::vector<std::unique_ptr<aggregation>> get_simple_aggregations(
Expand All @@ -118,7 +118,7 @@ class aggregation {
*/
class rolling_aggregation : public virtual aggregation {
public:
~rolling_aggregation() = default;
~rolling_aggregation() override = default;

protected:
rolling_aggregation() {}
Expand All @@ -130,7 +130,7 @@ class rolling_aggregation : public virtual aggregation {
*/
class groupby_aggregation : public virtual aggregation {
public:
~groupby_aggregation() = default;
~groupby_aggregation() override = default;

protected:
groupby_aggregation() {}
Expand All @@ -141,7 +141,7 @@ class groupby_aggregation : public virtual aggregation {
*/
class groupby_scan_aggregation : public virtual aggregation {
public:
~groupby_scan_aggregation() = default;
~groupby_scan_aggregation() override = default;

protected:
groupby_scan_aggregation() {}
Expand Down
8 changes: 4 additions & 4 deletions cpp/include/cudf/ast/detail/expression_evaluator.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ struct expression_result {
subclass().template set_value<Element>(index, result);
}

__device__ inline bool is_valid() const { return subclass().is_valid(); }
[[nodiscard]] __device__ inline bool is_valid() const { return subclass().is_valid(); }

__device__ inline T value() const { return subclass().value(); }
};
Expand Down Expand Up @@ -110,7 +110,7 @@ struct value_expression_result
/**
* @brief Returns true if the underlying data is valid and false otherwise.
*/
__device__ inline bool is_valid() const
[[nodiscard]] __device__ inline bool is_valid() const
{
if constexpr (has_nulls) { return _obj.has_value(); }
return true;
Expand Down Expand Up @@ -174,7 +174,7 @@ struct mutable_column_expression_result
/**
* @brief Not implemented for this specialization.
*/
__device__ inline bool is_valid() const
[[nodiscard]] __device__ inline bool is_valid() const
{
// Not implemented since it would require modifying the API in the parent class to accept an
// index.
Expand All @@ -186,7 +186,7 @@ struct mutable_column_expression_result
/**
* @brief Not implemented for this specialization.
*/
__device__ inline mutable_column_device_view value() const
[[nodiscard]] __device__ inline mutable_column_device_view value() const
{
// Not implemented since it would require modifying the API in the parent class to accept an
// index.
Expand Down
10 changes: 5 additions & 5 deletions cpp/include/cudf/ast/detail/expression_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ class expression_parser {
*
* @return cudf::data_type
*/
cudf::data_type output_type() const;
[[nodiscard]] cudf::data_type output_type() const;

/**
* @brief Visit a literal expression.
Expand Down Expand Up @@ -206,10 +206,10 @@ class expression_parser {
*/
class intermediate_counter {
public:
intermediate_counter() : used_values(), max_used(0) {}
intermediate_counter() : used_values() {}
cudf::size_type take();
void give(cudf::size_type value);
cudf::size_type get_max_used() const { return max_used; }
[[nodiscard]] cudf::size_type get_max_used() const { return max_used; }

private:
/**
Expand All @@ -221,10 +221,10 @@ class expression_parser {
*
* @return cudf::size_type Smallest value not already in the container.
*/
cudf::size_type find_first_missing() const;
[[nodiscard]] cudf::size_type find_first_missing() const;

std::vector<cudf::size_type> used_values;
cudf::size_type max_used;
cudf::size_type max_used{0};
};

expression_device_view device_expression_data; ///< The collection of data required to evaluate
Expand Down
Loading

0 comments on commit 276bcf4

Please sign in to comment.