-
Notifications
You must be signed in to change notification settings - Fork 883
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
Add clang-tidy
to libcudf
#9860
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9860 +/- ##
================================================
- Coverage 10.49% 10.40% -0.09%
================================================
Files 119 119
Lines 20305 20557 +252
================================================
+ Hits 2130 2139 +9
- Misses 18175 18418 +243
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of Python script cleanup
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small number of questions/suggestions, none are particularly blocking so I am not requesting changes. Thanks for taking on this massive task @codereport!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't give this a second round of review from top to bottom right now but the changes I've seen (and updates from my previous pass) look good to me! Great work @codereport.
Waiting on #10008 to unlock CI |
BTW in my experience I find the clang-tidy misses a lot of opportunities for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't review every change but I approve of the configuration and the changes I have seen.
rerun tests |
@gpucibot merge |
This PR runs [pyupgrade](https://github.com/asottile/pyupgrade) to modernize some syntax in the Python code base. (I am preparing a "better code" talk for our team that will reference this PR.) The kinds of modernizing changes that pyupgrade makes are similar in purpose to those made by clang-tidy (#9860), but for Python. These include things like: - Using literals for empty dicts `{}` and lists `[]`, because they are a bit faster and more readable than `dict()` or `list()` - Removing extra parentheses or braces wrapping generator expressions if not needed - Preferring format strings - Using canonical error names, e.g. `IOError` became an alias for `OSError` in Python 3.3. - Replacing the "new-style" class definitions like `class A(object):` with the simpler syntax `class A:` - Removing unnecessary `__future__` imports that were needed for Python 2 compatibility, like `print_function` and `division` - Using raw strings for regular expressions, to clarify intent and avoid extra escaping The [pyupgrade README](https://github.com/asottile/pyupgrade) has a long showcase of other rules. Notes for reviewers: - pyupgrade has a lot of opinions on how to use type annotations, and I'm not sure if I like all of them (some may have compatibility issues?). I have skipped over the type annotations for now, to set a more reasonable scope for this PR. See commit 54b16b9 (since reverted) for examples. Please let me know what you think about these changes and I may investigate them further in a follow-up PR! - I have not added this tool to our CI style checks via a pre-commit hook yet. That can be done in a follow-up PR if we agree that we would like these types of changes to be required in the future. - I used the argument `--py37-plus` to only perform changes that are compatible with Python 3.7+. - I can revert the changes to the vendored code in `versioneer.py` if desired. (It's pretty outdated anyway, it looks like we're using versioneer 0.18, released on January 1, 2017.) Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Charles Blackmon-Luca (https://github.com/charlesbluca) - AJ Schmidt (https://github.com/ajschmidt8) URL: #10141
This changeset is large, but it's not very substantial. It's all the automated fixes produced by clang-tidy using our script. The bulk of the changes are either adding `[[nodiscard]]` to many functions or changing const ref args to pass by value and then move in cases where the parameter is only used to set a value. There are also some places where clang-tidy preferred either more or less namespacing of objects depending on the current namespace. The goal is to enable clang-tidy in CI, which we made progress towards in #9860 but stalled in #10064. This PR contains the first set of changes that will required for such a check to pass. I've marked this PR as breaking because some of the functions now marked as `[[nodiscard]]` are public APIs, so if consumers were ignoring the return values they will now see warnings, and if they are compiling with warnings as errors then the builds will break. Contributes to #584 Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #15894
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
: Add .clang-tidy and fix clang-tidy warnings rmm#857cuml
: [REVIEW] Enable clang tidy on cuML c++ sources cuml#1945To do list:
.clang-tidy
filemodernize-
changescxxopts
changescxxopts
filebuild/_deps
directoriesSplitting out the following into a separate PR so we can get the changes merged for 22.02 (#10064):
[ ] Disableclang-diagnostic-errors/warnings
[ ] Fix include files being skipped[ ] Set up CI script[ ] Clean up python script