Skip to content

Conversation

@harrism
Copy link
Member

@harrism harrism commented Aug 31, 2021

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-*,
                  -cppcoreguidelines-macro-usage,
                  -llvm-header-guard,
                  -modernize-use-trailing-return-type,
                  -readability-named-parameter'

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

@github-actions github-actions bot added the cpp Pertains to C++ code label Aug 31, 2021
@github-actions github-actions bot added the CMake label Sep 7, 2021
@github-actions github-actions bot added the Python Related to RMM Python API label Sep 9, 2021
@harrism harrism requested a review from a team as a code owner September 9, 2021 01:03
@harrism
Copy link
Member Author

harrism commented Sep 15, 2021

rerun tests

@harrism harrism added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Sep 15, 2021
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Sep 16, 2021
There was an unneeded call to `device_uvector::release()` that did not use the return value. rapidsai/rmm#857 marks that function `[[nodiscard]]`, so removing this allows libcudf to compile after the RMM PR is merged.

The `release()` is unnecessary because `prev_base_offsets` is passed by rvalue reference and therefore will be freed when the function exits (the uvector in the calling context it has been moved-from).

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

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Jake Hemstad (https://github.com/jrhemstad)

URL: #9237
@harrism
Copy link
Member Author

harrism commented Sep 16, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 20eb64a into rapidsai:branch-21.10 Sep 16, 2021
rapids-bot bot pushed a commit that referenced this pull request Sep 17, 2021
#857 introduced a few naming issues in debug builds due to parameter name changes. I rebuilt both C++ and Cython-generated code in debug mode and ran tests, both build and tests succeed now.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - MithunR (https://github.com/mythrocks)
  - Rong Ou (https://github.com/rongou)
  - Jake Hemstad (https://github.com/jrhemstad)

URL: #875
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jan 20, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge CMake cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants