Skip to content

Enforce braces on control statements with clang-format - #1599

Merged
brminich merged 5 commits into
ai-dynamo:mainfrom
guy-ealey-morag:clang-format-braces
May 5, 2026
Merged

brminich merged 5 commits into
ai-dynamo:mainfrom
guy-ealey-morag:clang-format-braces

Conversation

@guy-ealey-morag

@guy-ealey-morag guy-ealey-morag commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

What?

Change clang-format rules to enforce brackets on control statements (if, else, for, do, while)

Why?

This is a repeated issue in NIXL PRs.
With this change CI will fail if any control statement is missing braces (in new and changed code only)

Examples

// fail
if (cond) NIXL_INFO << "single line without braces";

// fail
if (cond) { NIXL_INFO << "single line with braces"; }

// fail
if (cond)
    NIXL_INFO << "block without braces";

// pass
if (cond) {
    NIXL_INFO << "block with braces";
}

// fail
for (int i = 0; i < 10; i++) NIXL_INFO << "for loop without braces one line";

// fail
for (int i = 0; i < 10; i++)
    NIXL_INFO << "for loop without braces";

// pass
for (int i = 0; i < 10; i++) {
    NIXL_INFO << "for loop with braces";
}

Summary by CodeRabbit

  • Chores
    • Updated code formatting rules to enforce stricter, more consistent formatting across the codebase (automatic brace insertion and no single-line control statements).
  • Documentation
    • Updated coding style guidance to require braces for control statements and adjusted examples for empty blocks.

Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
@github-actions

Copy link
Copy Markdown

👋 Hi guy-ealey-morag! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

@guy-ealey-morag
guy-ealey-morag marked this pull request as ready for review April 29, 2026 12:10
@guy-ealey-morag
guy-ealey-morag requested a review from a team as a code owner April 29, 2026 12:10
@guy-ealey-morag

Copy link
Copy Markdown
Contributor Author

/build

@coderabbitai

coderabbitai Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8d5b8ee6-5e64-43de-8a61-4683d403e274

📥 Commits

Reviewing files that changed from the base of the PR and between 8b19200 and b39ceba.

📒 Files selected for processing (2)
  • .clang-format
  • docs/CodeStyle.md

📝 Walkthrough

Walkthrough

Updated formatting rules: .clang-format now disallows single-line if statements and enables automatic brace insertion; docs/CodeStyle.md was adjusted to reflect mandatory braces and updated empty-block examples. (46 words)

Changes

Cohort / File(s) Summary
Clang Format Config
\.clang-format
Changed AllowShortIfStatementsOnASingleLine from WithoutElse to Never; added InsertBraces: true.
Documentation / Code Style
docs/CodeStyle.md
Updated code-style guidance to require braces for control statements and adjusted empty-block examples to match the formatter rules.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hop through code with careful graces,
Braces snug in all their places,
No lone short if may dart or hide,
Formatter's rules now stem the tide,
I nibble bugs and tidy traces.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: enforcing braces on control statements using clang-format configuration.
Description check ✅ Passed The description follows the template with all required sections completed: What explains the clang-format rule changes, Why provides justification for the repeated issue, and Examples demonstrate the failing and passing scenarios.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.clang-format:
- Line 49: Add a short version note explaining the minimum required clang-format
version for the InsertBraces setting so contributors know to use
clang-format-15+; update the repo's .clang-format header (or CONTRIBUTING.md)
with a one-line comment like “Requires clang-format 15+ for InsertBraces” and/or
a CONTRIBUTING.md entry, and ensure the text references the InsertBraces setting
so developers running older clang-format binaries are warned before they format.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9d50c398-850f-4a96-893f-a36d2f4df80c

📥 Commits

Reviewing files that changed from the base of the PR and between ba5aead and 8b19200.

📒 Files selected for processing (1)
  • .clang-format

Comment thread .clang-format
tomerg-nvidia
tomerg-nvidia previously approved these changes Apr 29, 2026

@ColinNV ColinNV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very good, now the line

Short if-statements without else can be on single line when appropriate

in docs/CodeStyle.md can be removed.

tvegas1
tvegas1 previously approved these changes Apr 29, 2026
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
@guy-ealey-morag
guy-ealey-morag dismissed stale reviews from tvegas1 and tomerg-nvidia via b39ceba April 29, 2026 14:21
@guy-ealey-morag
guy-ealey-morag requested review from ColinNV and tvegas1 and removed request for tvegas1 April 29, 2026 14:21
@guy-ealey-morag

Copy link
Copy Markdown
Contributor Author

/build

@brminich
brminich merged commit f34ce83 into ai-dynamo:main May 5, 2026
16 checks passed
erezzarum pushed a commit to erezzarum/nixl that referenced this pull request May 24, 2026
)

---------

Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants