[None][doc] Update C++ coding guidelines.#12577
Conversation
Major changes: - Add struct vs class guidance based on invariants, with designated initializer and default member initializer recommendations - Add east-const rule (enforced by clang-format QualifierAlignment: Right) - Relax g/s prefix requirements for global and static variables from mandatory to encouraged; consolidate three rules into one with clear categories (true global, file-scope, function-local static) - Change constant naming convention from kUPPER_SNAKE_CASE to kCamelCase, matching the dominant codebase convention (~77 vs ~13 occurrences) - Add singleton accessor function-lcoal static pattern as preferred alternative to bare global/static variables. Required for non-POD constants - Add enum class guidance with naming conventions - Recommend #pragma once over traditional include guards - Allow return as switch case terminator - Add anonymous namespace, X-macro, bugprone-argument-comment examples - Note historical inconsistency; new code should follow guidelines Signed-off-by: Harris Nover <249353502+hnover-nv@users.noreply.github.com>
📝 WalkthroughWalkthroughUpdated C++ coding guidelines to enhance standards for namespaces, constants, formatting, casting, switch statements, struct/class usage, preprocessor directives, and pointer declarations. Changes include explicit tooling-alignment directives, expanded const requirements and naming conventions, an invariants-based framework for struct/class selection, and refined guidance on range-based loops and pointer qualifiers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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 `@CODING_GUIDELINES.md`:
- Around line 165-172: The docs claim clang-tidy's bugprone-argument-comment
enforces the /*paramName=*/ format but it isn't actually enforced: ensure the
.clang-tidy configuration enables StrictMode for that check (set
bugprone-argument-comment.StrictMode = true in CheckOptions) so comments without
the '=' are flagged, and optionally add clang-tidy to .pre-commit-config.yaml so
bugprone-argument-comment runs automatically on pre-commit; reference the
bugprone-argument-comment check name and the StrictMode option when making the
changes.
🪄 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: CHILL
Plan: Pro
Run ID: 9b40d93c-f667-4ffd-99ff-9fba3afad9e1
📒 Files selected for processing (1)
CODING_GUIDELINES.md
|
/bot run |
|
PR_Github #40670 [ run ] triggered by Bot. Commit: |
|
PR_Github #40670 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #40811 [ run ] triggered by Bot. Commit: |
|
PR_Github #40811 [ run ] completed with state |
Signed-off-by: Harris Nover <249353502+hnover-nv@users.noreply.github.com>
0f7080f to
bf586a4
Compare
|
/bot skip --comment "It passed in comment #12577 (comment) and it's just documentation anyway, so skipping" |
|
PR_Github #41901 Bot args parsing error: Failed to parse bot args |
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot skip --comment "It passed before and has a clean merge, and is just documentation, so skipping. Not sure why Jenkins wants it again, too old?" |
|
PR_Github #41944 [ skip ] triggered by Bot. Commit: |
|
PR_Github #41944 [ skip ] completed with state |
Signed-off-by: Harris Nover <249353502+hnover-nv@users.noreply.github.com>
Signed-off-by: Harris Nover <249353502+hnover-nv@users.noreply.github.com>
Signed-off-by: Harris Nover <249353502+hnover-nv@users.noreply.github.com>
Major changes:
Test Coverage
None
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit
#pragma oncesupport.