Conversation
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
Overview
Small, well-scoped PR that adds two CI steps:
- A
check-formatCMake target that verifies C++ files comply withclang-format(dry-run with--Werror) - A Ruff lint + format check step for the Python code in
gpu_test/
Also fixes the clang-format binary lookup on Ubuntu by searching for the versioned name (e.g. clang-format-20) first.
Analysis
Correctness — looks good overall.
-
clang-formatbinary search order (CMakeLists.txt): Searching forclang-format-${LLVM_VERSION_MAJOR}beforeclang-formatis the right call for Ubuntu where apt installs versioned binaries. Ensures the format check uses the same LLVM version as the build, avoiding style drift. -
CI step ordering (ci.yml): Format and lint checks run after build and test. This means a formatting-only failure still requires a full build cycle to surface. Consider moving these checks earlier or into a separate lightweight job — it doesn't need MLIR installed, so it could run on a bare image with just
clang-formatanduv. This would be faster and give clearer signal. -
check-formattarget has noclang-formatfallback (CMakeLists.txt): Ifclang-formatisn't found, bothformatandcheck-formatare silently unavailable. The CI step would then fail with a confusing "no rule to make target" error. Consider either:- Adding a
message(WARNING ...)when clang-format isn't found, or - Explicitly installing
clang-format-20in the CI apt step to make the dependency explicit rather than relying on it being pulled in transitively byllvm-20-dev.
- Adding a
-
Ruff step: Using
&&to chainruff checkandruff format --checkis fine — if lint fails, format won't run, which is reasonable.
Suggestions
- Consider a separate CI job for lint/format — doesn't need MLIR, would be much faster and gives independent signal.
- Install
clang-formatexplicitly in CI — addclang-format-${LLVM_VERSION}to theapt-get installline. - Minor:
file(GLOB_RECURSE ...)already recurses, so**in the glob paths is redundant (though harmless).
Verdict
Clean, minimal PR that adds useful CI guardrails. Main suggestion is a separate faster job for lint checks. Otherwise, LGTM.
Summary
check-formatCMake target that runsclang-format --dry-run --Werrorclang-format-20) on UbuntuTest plan
cmake --build build --target check-formatpasses locally