[mdboom]

This adds a pre-commit hook for clang-format, and then updates our C, C++ and CUDA code to follow that convention.

To be familiar for other developers of Python/C extensions, the formatting rules are based on PEP7, but deviating in 2 important ways:

• 120 line limit
• Line wrapping of function calls and declarations follows the black/ruff convention, where it either fits on one line, or it is line wrapped with a single argument per line. IMHO, line wrapping to fit columns is a confusing convention that causes poor diffs. This doesn't contradict PEP7, it merely extends it a little bit.

Follow-on

After this is merged, I plan to continue with clang-tidy as well.

Also after this is merged, we should add the merge commit's hash to .git-blame-ignore-revs so that git blame will skip over it (this is merely reformatting and generally would not be useful to a git blame query).

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: ab52ed1
• Latest commit, not reviewed: 65bedfb

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: NVIDIA/cuda-python
• Branch: main
• File: .coderabbit.yaml
• Commit: 9a01cb9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: a3529a20-4580-4778-b8b5-316c10e04a9d

📥 Commits

Reviewing files that changed from the base of the PR and between 5dd7ad5 and ab52ed1.

📒 Files selected for processing: 22

• .clang-format
• benchmarks/cuda_bindings/benchmarks/cpp/bench_ctx_device.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_event.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_launch.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_memory.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_stream.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_support.hpp
• cuda_core/cuda/core/_cpp/rt/api.hpp
• cuda_core/cuda/core/_cpp/rt/context.cpp
• cuda_core/cuda/core/_cpp/rt/context_scope.hpp
• cuda_core/cuda/core/_cpp/rt/driver_api.hpp
• cuda_core/cuda/core/_cpp/rt/error.hpp
• cuda_core/cuda/core/_cpp/rt/event.cpp
• cuda_core/cuda/core/_cpp/rt/graph.cpp
• cuda_core/cuda/core/_cpp/rt/graph_exec.cpp
• cuda_core/cuda/core/_cpp/rt/internal.hpp
• cuda_core/cuda/core/_cpp/rt/memory.cpp
• cuda_core/cuda/core/_cpp/rt/program.cpp
• cuda_core/cuda/core/_cpp/rt/py.hpp
• cuda_core/cuda/core/_cpp/rt/stream.cpp
• cuda_core/cuda/core/_cpp/rt/texture.cpp
• cuda_core/cuda/core/_cpp/rt/virtual_memory.cpp

🚧 Files skipped from review as they are similar to previous changes: 21

• benchmarks/cuda_bindings/benchmarks/cpp/bench_stream.cpp
• cuda_core/cuda/core/_cpp/rt/py.hpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_memory.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_launch.cpp
• cuda_core/cuda/core/_cpp/rt/driver_api.hpp
• cuda_core/cuda/core/_cpp/rt/internal.hpp
• cuda_core/cuda/core/_cpp/rt/graph_exec.cpp
• cuda_core/cuda/core/_cpp/rt/context_scope.hpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_ctx_device.cpp
• cuda_core/cuda/core/_cpp/rt/virtual_memory.cpp
• cuda_core/cuda/core/_cpp/rt/error.hpp
• cuda_core/cuda/core/_cpp/rt/texture.cpp
• cuda_core/cuda/core/_cpp/rt/program.cpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_support.hpp
• cuda_core/cuda/core/_cpp/rt/api.hpp
• benchmarks/cuda_bindings/benchmarks/cpp/bench_event.cpp
• cuda_core/cuda/core/_cpp/rt/event.cpp
• cuda_core/cuda/core/_cpp/rt/stream.cpp
• cuda_core/cuda/core/_cpp/rt/context.cpp
• cuda_core/cuda/core/_cpp/rt/memory.cpp
• cuda_core/cuda/core/_cpp/rt/graph.cpp

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

• Chores
  • Standardized C, C++ and CUDA formatting across the project.
  • Added contributor guidance and automatic formatting checks for supported source files.

Adds a pinned clang-format hook and repository formatting guidance. Applies the configured style to C++ and CUDA benchmark and runtime sources. The summaries report no changes to behavior, signatures, or benchmark operations.

Changes: C++ and CUDA formatting

Layer / File(s)	Summary
Formatting policy and setup .clang-format, .pre-commit-config.yaml, CONTRIBUTING.md	Adds the repository clang-format configuration, pins the formatter hook to version 23.1.3, and documents formatting instructions for C, C++, and CUDA files.
Benchmark source formatting benchmarks/cuda_bindings/benchmarks/cpp/*	Reformats benchmark implementations and shared helpers. Benchmark operations, arguments, error handling, and output behavior remain unchanged.
Runtime API and shared contracts cuda_core/cuda/core/_cpp/rt/api.hpp, cuda_core/cuda/core/_cpp/rt/{context_scope,driver_api,error,internal,types,versions}.*	Reformats runtime declarations and shared types. Signatures and behavior remain unchanged.
Runtime handle and resource implementations cuda_core/cuda/core/_cpp/rt/{context,event,graph,graph_exec,memory,program,stream,texture,virtual_memory}.*	Reformats runtime implementation code. Control flow, ownership, cleanup, and error handling remain unchanged.
Python integration and CUDA helpers cuda_core/cuda/core/_cpp/rt/py*, cuda_core/cuda/core/_include/{layout,utility}.hpp, cuda_core/tests/test_binaries/saxpy.cu	Reformats Python-coupled code, native-handle and layout helpers, and the SAXPY CUDA source. Signatures and behavior remain unchanged.

Suggested reviewers: jpascucci-nv

Priority: Low

Change: Other

Merge Risk: Minimal · previous review ab52e

The PR is mergeable as a formatting and tooling change.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Doc Preview CI
Preview removed because the pull request was closed or merged.

[rparolin]

This is where the line limit should inform the formatter to break the lambda across multiple lines the way it was before. Take a look at the clang-format settings I proposed in the issue ticket.

[mdboom]

This doesn't exceed the line limit of 120. I think what you may really be asking for is to always break inside of the {}?

[rparolin]

I don't mind a single short lambda on one line but its longer it should broken across multiple lines.

[rparolin]

This is another example where the format should break to a new line.

[mdboom]

I think the intention of the rule here is that single-line for loops are on the same line to prevent this kind of confusion:

for (int i = 0; i < 16; ++i) 
    packed_16[i] = &int_ptrs[i];
    packed_16[i] = &int_ptrs[i];

So the fix is to add explicit {}.

[rparolin]

This largely looks fine, but I would fix the lambda's so they are not all slammed onto a single line.

[Andy-Jost]

The formatting looks mostly better to me. Requesting changes the for inline lambda formatting. Other requests are optional.

[Andy-Jost]

I find the one-line versions of these functions easier to read.

[Andy-Jost]

I prefer the structured (before) version of this code. My actual top preference is the (horrors) comma-first approach, but that's unusual:

auto ret = function(
    first_arg
  , second_arg
  , third_arg(
        has_a
      , function_call
      )
  , [](int n) { // fourth arg
        with_lambda_stuff();
        return 0;
      }
  , fifth_arg
  );

Since there's no formatter setting to break at lambda headers, I'd prefer manual formatting with // clang-format off in sections like this (details in this comment).

[Andy-Jost]

Please fix this before merge. clang-format 23 has no option to break before a trailing lambda argument. When the call fits in 120 columns, the lambda header stays on the argument line, as here. When it does not fit, the call breaks into one argument per line. _cpp/rt/ now mixes both layouts.

Please fence each std::shared_ptr construction that takes a deleter lambda, and restore the layout from main:

    // clang-format off
    auto box = std::shared_ptr<DevicePtrBox>(
        new DevicePtrBox{ptr, std::move(ds)},
        [h_pool](DevicePtrBox* b) {
            GILReleaseGuard gil;
            const DeallocationStream& stream = b->deallocation;
            cleanup_in_context(
                deallocation_context(stream), "cuMemFreeAsync", handle_bits(b->resource),
                [&]() noexcept {
                    return DRIVER_CALL(cuMemFreeAsync, b->resource, as_cu(stream.h_stream));
                });
            delete b;
        }
    );
    // clang-format on

There are 34 such constructions in context.cpp, event.cpp, graph.cpp, memory.cpp, program.cpp, stream.cpp, texture.cpp, and virtual_memory.cpp. Of these, 26 changed to the argument-line form. Fence all 34 so the layer keeps one layout. The FileDescriptorHandle construction in create_fd_handle follows the same pattern.

I checked this site with the pinned 23.1.3. The hook leaves the fenced block and the rest of the file unchanged. Code inside a fence keeps the pointer style it is written in, so match the PointerAlignment you choose.

[mdboom]

Done

[Andy-Jost]

Optional, a slight preference for Left. Before this PR, _cpp/rt/ and the benchmarks wrote type* name in nearly every declaration, and Left keeps that style. On this branch, the switch changes 696 lines in 34 files. CPython and the PEP 7 examples use type *name, so keep Right if you prefer to stay with PEP 7.

Suggested change

	PointerAlignment: Right
	PointerAlignment: Left

[mdboom]

I think we should stick to PEP7 where we can. For C++ constructs like lambdas that it doesn't address, that's a judgement call for us, of course.

[Andy-Jost]

Optional, a slight preference for None. All also breaks after the return type in declarations. My comment on api.hpp shows the effect. PEP 7 puts the function name in column 1 only in definitions. With None, this branch has 690 fewer lines in 34 files. If you prefer to match PEP 7, TopLevelDefinitions does that.

The suggestion also uses the current key name, BreakAfterReturnType. If you change the value, update the header comment on lines 5 and 6.

Suggested change

	AlwaysBreakAfterReturnType: All
	BreakAfterReturnType: None

[mdboom]

Do you mean None or AllDefinitions here? It seems None is deprecated and puts it into a very confusing "Automatic" mode with rules that I'm not sure would be clear to anyone. I think AllDefinitions would put this in alignment with PEP7 and simplify all of the declarations as you said.

[Andy-Jost]

Optional. In clang-format 23, eight of these keys or values are deprecated or kept only for backward compatibility. The pinned 23.1.3 still reads them. If you want the current spellings:

Current	clang-format 23
AlwaysBreakAfterReturnType: All	BreakAfterReturnType: All
AllowShortIfStatementsOnASingleLine: false	AllowShortIfStatementsOnASingleLine: Never
ReflowComments: true	ReflowComments: Always
SpacesInParentheses: false	SpacesInParens: Never
SortIncludes: Never	SortIncludes: {Enabled: false}
BinPackArguments: false	PackArguments: {BinPack: OnePerLine}
BinPackParameters: false	PackParameters: {BinPack: OnePerLine}
AlignAfterOpenBracket: BlockIndent	the block below

AlignAfterOpenBracket: true
BreakAfterOpenBracketBracedList: true
BreakAfterOpenBracketFunction: true
BreakAfterOpenBracketIf: true
BreakBeforeCloseBracketBracedList: true
BreakBeforeCloseBracketFunction: true
BreakBeforeCloseBracketIf: true

The BlockIndent mapping comes from Format.cpp on the 23.x branch. I checked the full set with 23.1.3. --dump-config prints the same effective style for both files, and a reformat of all 37 files changes nothing.

The new keys need clang-format 22 or 23. An older clang-format in an editor stops on unknown keys by default. That is the case for keeping the current spellings.

[mdboom]

Done

[Andy-Jost]

Thanks for the cleanup!

[rparolin]

Is this intentional? Our formatting should be able to handle this block b/c its common pattern.

[rparolin]

For the most part the change is good. I'd like a future change to be made that remove the explicit clang format off/on on the common boxing pattern that we use.

[mdboom]

For the most part the change is good. I'd like a future change to be made that remove the explicit clang format off/on on the common boxing pattern that we use.

I'm not sure clang-format provides the knobs necessary to make that possible and have the syntax you and Andy want. We may have to compromise on some part of it...