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

[REVIEW] Enable clang tidy on cuML c++ sources #1945

Merged
merged 65 commits into from
May 24, 2020

Conversation

teju85
Copy link
Member

@teju85 teju85 commented Mar 29, 2020

  1. A simple run-clang-tidy.py wrapper script to invoke tidy checks on all compilation targets in the compilation database generated by cmake
  2. Enabled the generation of compile_commands.json file from cmake, by default.
  3. Only enabling a very minimal set of tidy rules in order to minimize the impact of this change. The plan is over the next few releases gradually enable more rules and clean-up the source files
  4. Fixes to the issues raised by the current tidy checks

Currently, not yet enabled the checks on .cu source files due to some wierd setup issues I'm facing. Hoping to fix those in the coming days.

@dantegd in order to enable this check from inside ci/checks/style.sh, I'll need to have run cmake before (in order to generate compilation database). But running cmake from style.sh will only cause the errors that are currently happening in PR #1931. Please suggest the right place (and the right way) in our CI to enable this check.

teju85 added 27 commits March 5, 2020 18:13
@teju85 teju85 requested review from a team as code owners March 29, 2020 02:41
@teju85 teju85 changed the title [REVIEW] Enable clang tidy on cuML c++ sources [skip-ci] [REVIEW] Enable clang tidy on cuML c++ sources May 9, 2020
@teju85 teju85 changed the title [skip-ci] [REVIEW] Enable clang tidy on cuML c++ sources [REVIEW] Enable clang tidy on cuML c++ sources May 9, 2020
@teju85
Copy link
Member Author

teju85 commented May 9, 2020

@JohnZed I have resolved the issues and gotten the check-style CI to pass. Can I get a re-review from you?

ci/checks/style.sh Show resolved Hide resolved
cpp/scripts/run-clang-tidy.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks great! Nice that it moved to style.sh too.

Copy link
Member

@raydouglass raydouglass left a comment

Choose a reason for hiding this comment

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

There must be a simpler way to do this check.

@raydouglass
Copy link
Member

rerun tests

@teju85
Copy link
Member Author

teju85 commented May 14, 2020

Thanks @raydouglass. Seems like this doesn't allow merging without @mike-wendt 's approval too. Can get one @mike-wendt ?

ci/checks/style.sh Outdated Show resolved Hide resolved
@teju85 teju85 changed the base branch from branch-0.14 to branch-0.15 May 20, 2020 16:06
@dantegd dantegd merged commit 534459c into rapidsai:branch-0.15 May 24, 2020
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
3 - Ready for Review Ready for review by team Build or Dep Issues related to building the code or dependencies CUDA / C++ CUDA issue Tech Debt Issues related to debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants