Skip to content

Commit

Permalink
Add .clang-tidy and fix clang-tidy warnings (#857)
Browse files Browse the repository at this point in the history
Adds a .clang-tidy configuration file.   This does not add CI tests yet, but I wanted to get it ready to do so without forcing it on anyone. As-is, this PR will enable clang-tidy "yellow squiggles" if you use clangd in vscode.

Starting with these Checks:

```
Checks:          'clang-diagnostic-*,
                  clang-analyzer-*,
                  cppcoreguidelines-*,
                  modernize-*,
                  bugprone-*,
                  performance-*,
                  readability-*,
                  llvm-*,
                  -modernize-use-trailing-return-type'
``` 

Here is a list of warnings fixed

* readability-identifier-length
* cppcoreguidelines-special-member-functions
* modernize-use-nodiscard
* modernize-use-nullptr
* cppcoreguidelines-pro-type-cstyle-cast (ignore locally for CUDA-defined stream constants)
* performance-noexcept-move-constructor
* readability-function-cognitive-complexity
* readability-qualified-auto
* modernize-concat-nested-namespaces
* readability-isolate-declaration
* readability-redundant-member-init
* modernize-use-override
* modernize-avoid-c-arrays
* cppcoreguidelines-pro-bounds-array-to-pointer-decay
* cppcoreguidelines-pro-bounds-constant-array-index
* bugprone-narrowing-conversions
* readability-implicit-bool-conversion
* readability-redundant-smartptr-get
* llvm-else-after-return
* readability-uppercase-literal-suffix
* readability-braces-around-statements
* cppcoreguidelines-avoid-magic-numbers
* performance-unnecessary-value-param
* bugprone-macro-parentheses
* performance-faster-string-find

NOLINT lines have been added where fixes are unavailable, such as with necessary pointer arithmetic or `reinterpret_cast` calls.

I have a private branch of google test which adds NOLINT lines to suppress warnings caused by gtest's code. I plan to make a PR for this.

There are still a couple of outstanding warnings having to do with "easily swappable parameters" (e.g. when a function takes two sizes.  Fixing these requires breaking API changes. Alternatively we could suppress them.)

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Rong Ou (https://github.com/rongou)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #857
  • Loading branch information
harrism authored Sep 16, 2021
1 parent fe53a72 commit 20eb64a
Show file tree
Hide file tree
Showing 70 changed files with 2,402 additions and 2,093 deletions.
67 changes: 67 additions & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
---
Checks: 'clang-diagnostic-*,
clang-analyzer-*,
cppcoreguidelines-*,
modernize-*,
bugprone-*,
performance-*,
readability-*,
llvm-*,
-cppcoreguidelines-macro-usage,
-llvm-header-guard,
-modernize-use-trailing-return-type,
-readability-named-parameter'
WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: none
CheckOptions:
- key: cert-dcl16-c.NewSuffixes
value: 'L;LL;LU;LLU'
- key: cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField
value: '0'
- key: cert-str34-c.DiagnoseSignedUnsignedCharComparisons
value: '0'
- key: cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors
value: '1'
- key: cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
value: '1'
- key: google-readability-braces-around-statements.ShortStatementLines
value: '1'
- key: google-readability-function-size.StatementThreshold
value: '800'
- key: google-readability-namespace-comments.ShortNamespaceLines
value: '10'
- key: google-readability-namespace-comments.SpacesBeforeComments
value: '2'
- key: llvm-else-after-return.WarnOnConditionVariables
value: '0'
- key: llvm-else-after-return.WarnOnUnfixable
value: '0'
- key: llvm-qualified-auto.AddConstToQualified
value: '0'
- key: modernize-loop-convert.MaxCopySize
value: '16'
- key: modernize-loop-convert.MinConfidence
value: reasonable
- key: modernize-loop-convert.NamingStyle
value: CamelCase
- key: modernize-pass-by-value.IncludeStyle
value: llvm
- key: modernize-replace-auto-ptr.IncludeStyle
value: llvm
- key: modernize-use-nullptr.NullMacros
value: 'NULL'
- key: readability-identifier-length.IgnoredParameterNames
value: 'mr|os'
- key: readability-identifier-length.IgnoredVariableNames
value: 'mr|_'
#- key: readability-function-cognitive-complexity.IgnoreMacros
# value: '1'
- key: bugprone-easily-swappable-parameters.IgnoredParameterNames
value: 'alignment'
- key: cppcoreguidelines-avoid-magic-numbers.IgnorePowersOf2IntegerValues
value: '1'
- key: readability-magic-numbers.IgnorePowersOf2IntegerValues
value: '1'
...
22 changes: 11 additions & 11 deletions benchmarks/cuda_stream_pool/cuda_stream_pool_bench.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, NVIDIA CORPORATION.
* Copyright (c) 2020-2021, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,36 +14,36 @@
* limitations under the License.
*/

#include <benchmark/benchmark.h>

#include <rmm/cuda_stream_pool.hpp>
#include <rmm/detail/error.hpp>

#include <cuda_runtime_api.h>

#include <benchmark/benchmark.h>

#include <stdexcept>

static void BM_StreamPoolGetStream(benchmark::State& state)
{
rmm::cuda_stream_pool stream_pool{};

for (auto _ : state) {
auto s = stream_pool.get_stream();
cudaStreamQuery(s.value());
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
auto stream = stream_pool.get_stream();
cudaStreamQuery(stream.value());
}

state.SetItemsProcessed(state.iterations());
state.SetItemsProcessed(static_cast<int64_t>(state.iterations()));
}
BENCHMARK(BM_StreamPoolGetStream)->Unit(benchmark::kMicrosecond);

static void BM_CudaStreamClass(benchmark::State& state)
{
for (auto _ : state) {
auto s = rmm::cuda_stream{};
cudaStreamQuery(s.view().value());
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
auto stream = rmm::cuda_stream{};
cudaStreamQuery(stream.view().value());
}

state.SetItemsProcessed(state.iterations());
state.SetItemsProcessed(static_cast<int64_t>(state.iterations()));
}
BENCHMARK(BM_CudaStreamClass)->Unit(benchmark::kMicrosecond);

Expand Down
26 changes: 14 additions & 12 deletions benchmarks/device_uvector/device_uvector_bench.cu
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, NVIDIA CORPORATION.
* Copyright (c) 2020-2021, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,33 +14,35 @@
* limitations under the License.
*/

#include <benchmark/benchmark.h>

#include <cuda_runtime_api.h>
#include <rmm/device_uvector.hpp>
#include <rmm/device_vector.hpp>
#include <rmm/mr/device/cuda_memory_resource.hpp>
#include <rmm/mr/device/per_device_resource.hpp>
#include <rmm/mr/device/pool_memory_resource.hpp>

#include <benchmark/benchmark.h>

#include <cuda_runtime_api.h>

static void BM_UvectorSizeConstruction(benchmark::State& state)
{
rmm::mr::cuda_memory_resource cuda_mr{};
rmm::mr::pool_memory_resource<rmm::mr::cuda_memory_resource> mr{&cuda_mr};
rmm::mr::set_current_device_resource(&mr);

for (auto _ : state) {
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
rmm::device_uvector<int32_t> vec(state.range(0), rmm::cuda_stream_view{});
cudaDeviceSynchronize();
}

state.SetItemsProcessed(state.iterations());
state.SetItemsProcessed(static_cast<int64_t>(state.iterations()));

rmm::mr::set_current_device_resource(nullptr);
}

BENCHMARK(BM_UvectorSizeConstruction)
->RangeMultiplier(10)
->Range(10'000, 1'000'000'000)
->RangeMultiplier(10) // NOLINT
->Range(10'000, 1'000'000'000) // NOLINT
->Unit(benchmark::kMicrosecond);

static void BM_ThrustVectorSizeConstruction(benchmark::State& state)
Expand All @@ -49,19 +51,19 @@ static void BM_ThrustVectorSizeConstruction(benchmark::State& state)
rmm::mr::pool_memory_resource<rmm::mr::cuda_memory_resource> mr{&cuda_mr};
rmm::mr::set_current_device_resource(&mr);

for (auto _ : state) {
for (auto _ : state) { // NOLINT(clang-analyzer-deadcode.DeadStores)
rmm::device_vector<int32_t> vec(state.range(0));
cudaDeviceSynchronize();
}

state.SetItemsProcessed(state.iterations());
state.SetItemsProcessed(static_cast<int64_t>(state.iterations()));

rmm::mr::set_current_device_resource(nullptr);
}

BENCHMARK(BM_ThrustVectorSizeConstruction)
->RangeMultiplier(10)
->Range(10'000, 1'000'000'000)
->RangeMultiplier(10) // NOLINT
->Range(10'000, 1'000'000'000) // NOLINT
->Unit(benchmark::kMicrosecond);

BENCHMARK_MAIN();
Loading

0 comments on commit 20eb64a

Please sign in to comment.