Repository navigation
Refactor libcudacxx-style skill by moving CCCL wide style guidelines to a specific file - #9405
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
OverviewThis PR refactors the libcudacxx-style agent skill by extracting CCCL-wide style guidance into shared common documentation and moving libcudacxx-specific style guidance into a separate referenced document. The reorganization ensures LLM reviewers apply rules only within the intended CCCL subproject scope. ChangesNew Agent Skills Structure
New Test Skill Structure
Configuration Updates
Metadata Updates
Deprecated
ImpactBy separating CCCL-wide guidelines from libcudacxx-specific rules, agents/LLMs should stop generating incorrect style suggestions for other CCCL components (e.g., preventing guidance meant for one subproject from being applied to another). WalkthroughReplace single libcudacxx-style skill with hierarchical cccl-style skill (shared common rules plus libcudacxx-specific reference), add cccl-test skill and references, update CodeRabbit configuration and review instructions, and update agent metadata and AGENTS.md to reference new skills. ChangesCCCL Style Guidance Reorganization
Possibly related issues
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a04d42b-2b09-4367-97c6-5b93ba0e793e
📒 Files selected for processing (6)
.agent/skills/cccl-style/SKILL.md.agent/skills/cccl-style/references/common.md.agent/skills/cccl-style/references/libcudacxx.md.agent/skills/libcudacxx-style/SKILL.md.coderabbit.yamlAGENTS.md
💤 Files with no reviewable changes (1)
- .agent/skills/libcudacxx-style/SKILL.md
fbusato
left a comment
fbusato
left a comment
There was a problem hiding this comment.
I like the idea to unify and organize SKILL styles. A couple of comments:
- https://github.com/NVIDIA/cccl/blob/main/.agent/skills/libcudacxx-test/SKILL.md is missing
- "local using declaration to avoid repetition" is missing
- namespace qualification: "current namespace or an enclosing one" is missing
- free function namespace qualification example is missing
fbusato
left a comment
fbusato
left a comment
There was a problem hiding this comment.
a few suggestions (if makes sense) to move common rules into common.md
|
precommit.ci autofix |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aad766ae-1356-4dc1-9821-e94151ba6358
📒 Files selected for processing (2)
.agent/skills/cccl-style/references/common.md.agent/skills/cccl-style/references/libcudacxx.md
💤 Files with no reviewable changes (1)
- .agent/skills/cccl-style/references/libcudacxx.md
|
pre-commit.ci autofix |
|
/ok to test 64ffdc7 |
|
|
||
| ## Function Calls And Types | ||
|
|
||
| - In headers, free function calls must be fully qualified from the global namespace, e.g. `::cuda::ceil_div(...)`. This includes calls to functions defined in the same namespace, e.g. inside `cuda::`, call `::cuda::ceil_div(...)`, not `ceil_div(...)`. This does not apply to (static) member functions of classes. The only exceptions to this rule are functions that are supposed to be found through argument-dependent lookup (ADL), such as `::cuda::std::swap` and `::cuda::std::get`. Those functions can be called unqualified with a preceding `using ::cuda::std::get;`. |
There was a problem hiding this comment.
Important: This does not apply to Thrust, which relies on ADL in many places. It also only partially applies to CUB, where full qualification is only required for symbols in the ::cuda::* namespace.
There was a problem hiding this comment.
ADL is covered by this rule. Also, what do you mean for "partially applies"? function/variable/both?
|
|
||
| - In headers, free function calls must be fully qualified from the global namespace, e.g. `::cuda::ceil_div(...)`. This includes calls to functions defined in the same namespace, e.g. inside `cuda::`, call `::cuda::ceil_div(...)`, not `ceil_div(...)`. This does not apply to (static) member functions of classes. The only exceptions to this rule are functions that are supposed to be found through argument-dependent lookup (ADL), such as `::cuda::std::swap` and `::cuda::std::get`. Those functions can be called unqualified with a preceding `using ::cuda::std::get;`. | ||
| - This global-qualification rule does not apply to source files such as tests and benchmarks. | ||
| - Type names must be fully qualified except when they are already declared in the current namespace or an enclosing one. |
There was a problem hiding this comment.
Same comment as above regarding Thrust and CUB.
| - libcudacxx and cudax require free function calls to be fully qualified from the global namespace, e.g. `::cuda::ceil_div(...)`. | ||
| - CUB applies this rule only to calls to symbols under the `::cuda` namespace hierarchy; otherwise follow existing CUB qualification style. | ||
| - Thrust uses leading `::` for many symbols under the `::cuda` namespace hierarchy, but relies on ADL in many places and the blanket free-function qualification rule does not apply to those calls. | ||
| - For covered calls, this includes calls to functions defined in the same namespace, e.g. inside `cuda::`, call `::cuda::ceil_div(...)`, not `ceil_div(...)`. This does not apply to (static) member functions of classes. The only exceptions for covered calls are functions that are supposed to be found through argument-dependent lookup (ADL), such as `::cuda::std::swap` and `::cuda::std::get`. Those functions can be called unqualified with a preceding `using ::cuda::std::get;`. |
There was a problem hiding this comment.
It's actually more complex than this, because Thrust uses ADL also for backend system selection, but this should definitely go to a Thrust skill eventually.
The details are provided here: https://nvidia.github.io/cccl/unstable/thrust/developer/systems.html#algorithm-dispatch
There was a problem hiding this comment.
Created #9466 to look into the interaction between developer documentation and internal code writing + reviewing skills
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.agent/skills/cccl-style/references/common.md (1)
17-17: 💤 Low valuesuggestion: "builtin" should be "built-in" (hyphenated) per modern English style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2ed2857a-3aa8-4f5f-b02e-1e881f844bac
📒 Files selected for processing (1)
.agent/skills/cccl-style/references/common.md
|
/ok to test 2ad49f7 |
This PR refactors the libcudacxx-style SKILL.md by moving global CCCL style guidelines to a common file and libcudacxx specific style guidelines to its own file. The goal is to prevent comments from LLM reviewers such as this one #9347 (comment), where it suggested changes in CUB that did not apply