Repository navigation
Add clang-tidy to pre-commit #3250
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # Layout is clang-format's job (see .clang-format), so clang-tidy does no | ||
| # formatting of its own (FormatStyle: none below). Each enabled check is meant | ||
| # to be adoptable on its own, so findings can be fixed one check at a time. | ||
| # | ||
| # Checks are opt-in (-*) by group, minus the ones below. They cannot be | ||
| # commented inline because Checks is a single YAML string. | ||
| # | ||
| # Disabled because they conflict with the C-style CUDA driver API idioms: | ||
| # bugprone-easily-swappable-parameters: wrappers of driver calls take many | ||
| # adjacent parameters of the same type; the signatures mirror the API. | ||
| # bugprone-assignment-in-if-condition: `if (CUDA_SUCCESS != (err = CALL(...)))` | ||
| # is the standard error-handling pattern here. | ||
| # performance-no-int-to-ptr: opaque handles and file descriptors are | ||
| # converted to pointers with reinterpret_cast on purpose. | ||
| # modernize-avoid-c-arrays: arrays are passed to and filled by C APIs. | ||
| # modernize-return-braced-init-list: `return {};` vs. an explicit type is | ||
| # left to the author; it adds noise for the driver-API result structs. | ||
| # bugprone-throwing-static-initialization: the handle registries are | ||
| # process-lifetime globals whose constructors allocate on some standard | ||
| # libraries (MSVC); running out of memory while loading the module is | ||
| # fatal either way. | ||
| # | ||
| # Disabled because they do not match the codebase style: | ||
| # performance-enum-size: enum underlying types are left as they are. | ||
| # misc-const-correctness: const on every local is not the convention here. | ||
| # readability-function-cognitive-complexity: too disruptive. | ||
| # readability-identifier-length: short names such as the handle parameter `h` | ||
| # are used throughout. | ||
| # readability-implicit-bool-conversion: `if (ptr)` and `if (handle)` are the | ||
| # idiom for pointers and handles. | ||
| # misc-include-cleaner: too noisy with the CUDA and CPython umbrella headers. | ||
| # misc-no-recursion: recursion is used deliberately. | ||
| # misc-use-anonymous-namespace: file-local functions use `static`. | ||
| # modernize-use-trailing-return-type: the code uses `Type\nname()` layout. | ||
| Checks: > | ||
| -*, | ||
| bugprone-*, | ||
| -bugprone-easily-swappable-parameters, | ||
| -bugprone-assignment-in-if-condition, | ||
| -bugprone-throwing-static-initialization, | ||
| clang-analyzer-*, | ||
| misc-*, | ||
| -misc-include-cleaner, | ||
| -misc-const-correctness, | ||
| -misc-no-recursion, | ||
| -misc-use-anonymous-namespace, | ||
| modernize-*, | ||
| -modernize-use-trailing-return-type, | ||
| -modernize-avoid-c-arrays, | ||
| -modernize-return-braced-init-list, | ||
| performance-*, | ||
| -performance-enum-size, | ||
| -performance-no-int-to-ptr, | ||
| readability-*, | ||
| -readability-function-cognitive-complexity, | ||
| -readability-identifier-length, | ||
| -readability-implicit-bool-conversion | ||
| FormatStyle: none | ||
| CheckOptions: | ||
| # Records whose members are all public (the *Box and guard structs) are plain | ||
| # data holders; only a mix of public and private members is flagged. | ||
| misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic: true | ||
| WarningsAsErrors: '*' | ||
| # Only report diagnostics for our own headers, not CUDA/CPython/libstdc++. | ||
| HeaderFilterRegex: 'cuda_core/cuda/core/_(cpp|include)/' | ||
| ExcludeHeaderFilterRegex: 'cuda_core/cuda/core/_include/(dlpack|aoti_shim)\.h' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,14 @@ repos: | |
|
|
||
| - repo: local | ||
| hooks: | ||
| # cuda.h and nvrtc.h come from pinned nvidia-cuda-* wheels, so no local CUDA Toolkit is needed. | ||
| - id: clang-tidy | ||
| name: clang-tidy | ||
| entry: python ./toolshed/run_clang_tidy.py --cuda-wheel | ||
| language: python | ||
| files: ^cuda_core/cuda/core/_cpp/.*\.cpp$ | ||
| additional_dependencies: [clang-tidy==22.1.8, nvidia-cuda-runtime==13.4.92, nvidia-cuda-nvrtc==13.4.92] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This blocks every Mac developer from committing to the repo, including docs-only and Python-only changes. Could we add environment markers to these three dependencies, as the config already does for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. It's a shame there is no way to install the headers only.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This point makes me think (as I contemplated in my OP) that doing this in pre-commit is not the right way to go, since setting up a working development environment across all the configs is so fiddly. I think I'd rather have only one set of build environment setup (the one used for the actual build) and piggy-back on that. Plus, then you are sure you are linting with all of the exact same settings as you are building in. I think it probably makes sense to rework this as a setuptools extension. WDYT, @Andy-Jost? |
||
|
|
||
| - id: check-precommit-installed | ||
| name: Warn if the pre-commit git hook is not installed | ||
| entry: python ./toolshed/check_precommit_installed.py | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ Thank you for your interest in contributing to CUDA Python! Based on the type of | |
| - [Review Guidance](#review-guidance) | ||
| - [Pull Request Reviews](#pull-request-reviews) | ||
| - [Pixi lockfiles](#pixi-lockfiles) | ||
| - [Secret Scanning](#secret-scanning) | ||
| - [Signing Your Work](#signing-your-work) | ||
| - [Code signing](#code-signing) | ||
| - [Developer Certificate of Origin (DCO)](#developer-certificate-of-origin-dco) | ||
|
|
@@ -247,6 +248,12 @@ hook environment, so no system-wide installation is required. Vendored code | |
| `.pre-commit-config.yaml`. To format in your editor, install the same version | ||
| with `pip install clang-format==23.1.3`. | ||
|
|
||
| `cuda_core`'s C++ sources are also checked with `clang-tidy`, configured by | ||
| [`.clang-tidy`](.clang-tidy). To apply automatic fixes, run the script directly | ||
| with clang-tidy's `--fix` option (without `--cuda-wheel` it uses `CUDA_PATH` or | ||
| `CUDA_HOME`) and then run clang-format on the result: `python | ||
| toolshed/run_clang_tidy.py --fix <files>`. | ||
|
Comment on lines
+252
to
+255
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Document the tool needed for direct runs. The pre-commit hook installs
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. The command also omits There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Andy-Jost suggestion: Document installation of python toolshed/run_clang_tidy.py --cuda-wheel --fix <files>Keep the local-toolkit command as an explicit alternative, with a note that diagnostics can differ by toolkit version. You are interacting with an AI system. |
||
|
|
||
| ## CodeRabbit | ||
|
|
||
| This section explains how to use CodeRabbit for CUDA Python pull request review. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,9 +83,10 @@ struct GraphBox : GraphBoxBase { | |
| GraphAttachmentMap attachments; // Non-owning attachment index. | ||
| HandleRegistry<CUgraphNode, GraphNodeHandle> node_handles; | ||
|
|
||
| // Not noexcept: the std::map and HandleRegistry members allocate. | ||
| GraphBox( | ||
| CUgraph resource_, GraphHierarchy *hierarchy_, GraphBox *parent_ = nullptr, CUgraphNode owner_node_ = nullptr | ||
| ) noexcept | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| : GraphBoxBase{resource_}, hierarchy(hierarchy_), parent(parent_), owner_node(owner_node_) | ||
| { | ||
| } | ||
|
|
@@ -106,7 +107,7 @@ struct GraphHierarchy { | |
|
|
||
| // See REGISTRY_DESIGN.md (Level 1: Driver Handle -> Resource Handle) | ||
| using GraphRegistry = HandleRegistry<CUgraph, GraphHandle>; | ||
| static GraphRegistry graph_registry; | ||
| GraphRegistry graph_registry; | ||
|
|
||
| // Immutable resource owners for one version of a graph node's parameters. | ||
| // Inheriting DeferredCleanupItem lets CUDA's user-object destructor enqueue | ||
|
|
@@ -140,7 +141,7 @@ free_deleter(const void *p) noexcept | |
| GraphBox * | ||
| get_box(const GraphHandle &h) noexcept | ||
| { | ||
| auto *value = reinterpret_cast<const GraphBoxBase *>(h.get()); | ||
| const auto *value = reinterpret_cast<const GraphBoxBase *>(h.get()); | ||
| return const_cast<GraphBox *>(static_cast<const GraphBox *>(value)); | ||
| } | ||
|
|
||
|
|
@@ -254,7 +255,7 @@ struct PreparedAttachmentState { | |
| explicit PreparedAttachmentState(GraphHandle h_graph_) : h_graph(std::move(h_graph_)) {} | ||
| }; | ||
|
|
||
| void | ||
| static void | ||
| rollback_prepared_attachment(PreparedAttachmentState *state) noexcept | ||
| { | ||
| if (!state) { | ||
|
|
@@ -290,6 +291,25 @@ struct PreparedChildGraphUpdateState { | |
| } | ||
| }; | ||
|
|
||
| // Deleter of the shared hierarchy: it owns the root CUgraph once the root box | ||
| // exists. | ||
| static void | ||
| release_graph_hierarchy(GraphHierarchy *hierarchy) | ||
| { | ||
| for (const GraphBox &box : hierarchy->graphs) { | ||
| if (box.resource) { | ||
| graph_registry.unregister_handle(box.resource); | ||
| } | ||
| } | ||
| GraphBox *root = hierarchy->root(); | ||
| if (root && root->resource) { | ||
| GILReleaseGuard gil; | ||
| pw_cuGraphDestroy(root->resource); | ||
| } | ||
| retry_deferred_cleanup(); | ||
| delete hierarchy; | ||
| } | ||
|
|
||
| GraphHandle | ||
| create_graph_handle(CUgraph graph) | ||
| { | ||
|
|
@@ -298,28 +318,21 @@ create_graph_handle(CUgraph graph) | |
| return {}; | ||
| } | ||
|
|
||
| // clang-format off | ||
| auto hierarchy = std::shared_ptr<GraphHierarchy>( | ||
| new GraphHierarchy{}, | ||
| [](GraphHierarchy *hierarchy) { | ||
| for (const GraphBox& box : hierarchy->graphs) { | ||
| if (box.resource) { | ||
| graph_registry.unregister_handle(box.resource); | ||
| } | ||
| } | ||
| GraphBox *root = hierarchy->root(); | ||
| if (root && root->resource) { | ||
| GILReleaseGuard gil; | ||
| pw_cuGraphDestroy(root->resource); | ||
| } | ||
| retry_deferred_cleanup(); | ||
| delete hierarchy; | ||
| } | ||
| ); | ||
| // clang-format on | ||
| GraphBox &root = hierarchy->graphs.emplace_back(graph, hierarchy.get()); | ||
| // Until the root box exists the hierarchy deleter has nothing to destroy, | ||
| // so an allocation failure before then must release the graph here. | ||
| std::shared_ptr<GraphHierarchy> hierarchy; | ||
| GraphBox *root = nullptr; | ||
| try { | ||
| hierarchy = std::shared_ptr<GraphHierarchy>(new GraphHierarchy{}, release_graph_hierarchy); | ||
| root = &hierarchy->graphs.emplace_back(graph, hierarchy.get()); | ||
| } | ||
| catch (...) { | ||
| GILReleaseGuard gil; | ||
| pw_cuGraphDestroy(graph); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the stream-capture path this graph is not ours to destroy. Before this PR, an allocation failure here leaked the graph, which is what
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in both cases my agent and CodeRabbit suggested this change because of the |
||
| throw; | ||
|
Comment on lines
+321
to
+332
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| GraphHandle h_graph(hierarchy, &root.resource); | ||
| GraphHandle h_graph(hierarchy, &root->resource); | ||
| graph_registry.register_handle(graph, h_graph); | ||
| return h_graph; | ||
| } | ||
|
|
@@ -389,7 +402,7 @@ graph_prepare_child_graph_update( | |
| return CUDA_SUCCESS; | ||
| } | ||
|
|
||
| void | ||
| static void | ||
| publish_child_graph_update(PreparedChildGraphUpdateState &state, GraphHandle *out_child) | ||
| { | ||
| GraphBox *parent = get_box(state.h_parent); | ||
|
|
||
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.
With
fileslimited to.cppfiles under_cpp/, a change to a header (_cpp/rt/*.hpp,_include/), to.clang-tidy, or totoolshed/run_clang_tidy.pyskips the hook locally and then fails in CI's--all-filesrun. Could we widen the pattern to those paths and have the runner analyze every translation unit when it gets a non-.cpppath? A full run is cheap: the pre-commit jobs on this PR, which analyze every translation unit, finish within about a minute of the same jobs onmain.