[CI][Lint] Retire format.sh and add clang-tidy to GHA workflow
#1044
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
WalkthroughReplaces CI format-check with a Linux-focused clang-tidy job (fresh CMake build, discover sources, run run-clang-tidy/clang-tidy with optional auto-fix), restructures .clang-tidy, bumps clang-format/clang-tidy and pre-commit hook versions, adds cmake-build to .gitignore, refactors format.sh for merge-base/pre-commit modes, and applies many small style/const-ref tweaks across src/. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Job as CI Job
participant CMake as CMake (cmake-build)
participant Wrapper as run-clang-tidy / wrapper
participant Tidy as clang-tidy
participant Git as git
Dev->>GH: push / open PR
GH->>Job: start workflow
Job->>Job: prepare env (install LLVM/tooling if needed)
Job->>CMake: cmake -S . -B cmake-build --fresh (apply CLANG_TIDY_CMAKE_OPTIONS)
Job->>Job: discover C/C++ sources under src/
alt run-clang-tidy present
Job->>Wrapper: run-clang-tidy -clang-tidy-binary ... -p cmake-build <files> [--fix]
else download wrapper
Job->>Job: download run-clang-tidy.py
Job->>Wrapper: python run-clang-tidy.py -clang-tidy-binary ... -p cmake-build <files> [--fix]
end
Wrapper->>Tidy: spawn clang-tidy on discovered files
alt clang-tidy exits non-zero
Wrapper->>Git: git --no-pager diff --color=always
Job-->>GH: fail with clang-tidy exit code
else success
Job-->>GH: pass
end
Job->>Job: cleanup cmake-build/ and run-clang-tidy.py
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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 details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)format.sh(0 hunks)
💤 Files with no reviewable changes (1)
- format.sh
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.clang-tidy (1)
10-58: Checks block includes inline comments; clang-tidy will treat them as unknown check namesLines starting with “# …” are part of the folded string, not YAML comments. clang-tidy will parse them as tokens and likely fail. Remove the comment lines from within the Checks string (keep comments outside the scalar).
Apply this diff:
Checks: >- - # 1. Retained categories: easier to find bugs/performance issues clang-analyzer-*, cppcoreguidelines-pro-type-static-cast-downcast, cppcoreguidelines-pro-type-member-init, cppcoreguidelines-pro-bounds-array-to-pointer-decay, cppcoreguidelines-pro-bounds-pointer-arithmetic, cppcoreguidelines-slicing, cppcoreguidelines-narrowing-conversions, performance-*, - - # 2. Readability: only keep useful rules readability-braces-around-statements, readability-container-size-empty, readability-delete-null-pointer, readability-redundant-member-init, readability-redundant-smartptr-get, readability-redundant-string-cstr, - - # 3. Disable all intrusive/style-breaking rules -readability-identifier-length, -readability-avoid-const-params-in-decls, -readability-else-after-return, -cppcoreguidelines-avoid-magic-numbers, -modernize-use-trailing-return-type, -modernize-use-nodiscard, -modernize-use-auto, -modernize-pass-by-value, -modernize-return-braced-init-list, -modernize-use-default-member-init, -modernize-loop-convert, -modernize-concat-nested-namespaces, -llvm-include-order, -bugprone-unused-return-value, -clang-diagnostic-unused-result, -cppcoreguidelines-special-member-functions, -performance-noexcept-move-constructor, -cppcoreguidelines-narrowing-conversions, -clang-diagnostic-error, -cppcoreguidelines-pro-type-member-init, -clang-analyzer-optin.cplusplus.UninitializedObject, -cppcoreguidelines-pro-type-static-cast-downcast, -performance-unnecessary-value-param, -performance-enum-size, -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -clang-analyzer-deadcode.DeadStores, -clang-analyzer-optin.cplusplus.VirtualCall, -clang-diagnostic-tautological-constant-compare,
🧹 Nitpick comments (4)
.clang-tidy (4)
49-51: Conflicting toggles disable key safety checksYou enable these earlier but later disable them; last-match-wins means they end up disabled:
- cppcoreguidelines-pro-type-member-init (Line 49)
- cppcoreguidelines-pro-type-static-cast-downcast (Line 51)
- cppcoreguidelines-narrowing-conversions (Line 47)
Is this intentional? If not, drop the later “-…” entries.
7-7: HeaderFilterRegex is too broad; may exclude legitimate paths containing “build”Pattern excludes any path with substring “build” (e.g., src/builders/…). Prefer segment-based exclusion and the actual build dir name.
Apply this tweak:
-HeaderFilterRegex: '^(?!.*(3rdparty|build)).*$' +HeaderFilterRegex: '^(?!.*(?:^|/)(?:3rdparty|cmake-build)(?:/|$)).*$'
6-6: WarningsAsErrors: "*" will fail CI on any warningGood for strictness, but brittle for large diffs and third‑party headers that slip through. Confirm this is intended now that format.sh is retired.
Option: narrow to high-signal checks (e.g., “clang-analyzer-,performance-”) or gate via CI env to allow temporary soft-fail.
3-3: ExtraArgs: ['-v'] increases CI noiseVerbose compilation output can flood logs. Consider dropping “-v” unless needed for debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.clang-tidy(1 hunks).github/workflows/ci.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
XuehaiPan
force-pushed
the
retire-format-sh
branch
from
9d915d2 to
def09a4
Compare
XuehaiPan
changed the title
format.sh and add clang-tidy to GHA workflowformat.sh and add clang-tidy to GHA workflow
XuehaiPan
force-pushed
the
retire-format-sh
branch
5 times, most recently
from
b7fbeb0 to
b4b454d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
320-324: Restrict clang-tidy inputs to translation units from compile_commands.json (and add parallelism).Current find:
- Includes headers and misses proper grouping, risking false positives and failures.
- Prior review already flagged headers in the list causing clang-tidy errors. Use compile_commands.json instead and parallelize runs.
- CXX_FILES=$(find src -type f -iname "*.[ch]pp" -o -iname "*.cc" -o -iname "*.c" -o -iname "*.h") - rc=0 - "${CLANG_TIDY[@]}" -clang-tidy-binary="$(command -v clang-tidy)" \ - -p="cmake-build" ${CXX_FILES} || rc="$?" + # Extract translation units from compile_commands.json (exclude headers) + mapfile -t CXX_FILES < <(jq -r '.[].file' cmake-build/compile_commands.json | \ + awk '/\.(c|cc|cpp|cxx|cu|m|mm)$/ {print}' | sort -u) + if (( ${#CXX_FILES[@]} == 0 )); then + echo "::warning::No translation units found in compile_commands.json; skipping clang-tidy." + exit 0 + fi + # Determine reasonable parallelism + if command -v nproc >/dev/null; then JOBS="$(nproc)"; else JOBS="$(sysctl -n hw.ncpu 2>/dev/null || echo 2)"; fi + rc=0 + "${CLANG_TIDY[@]}" -j "${JOBS}" -clang-tidy-binary="$(command -v clang-tidy)" \ + -p="cmake-build" "${CXX_FILES[@]}" || rc="$?"If jq may be missing on some self-hosted runners, consider adding a minimal jq install or a small Python fallback. Based on learnings
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
229-239: Guard Homebrew ops and export only if brew exists (Metal runners).On non-GitHub-hosted/mac runners without Homebrew, these commands will fail. Add a guard and quieter install.
- brew update --quiet --force - brew list --formula llvm >/dev/null || brew install --formula llvm - export PATH="$(brew --prefix llvm)/bin:${PATH}" - export CPPFLAGS="-I$(brew --prefix llvm)/include${CPPFLAGS:+ ${CPPFLAGS}}" + if command -v brew >/dev/null; then + brew update --quiet --force + brew list --formula llvm >/dev/null || brew install --quiet --formula llvm + export PATH="$(brew --prefix llvm)/bin:${PATH}" + export CPPFLAGS="-I$(brew --prefix llvm)/include${CPPFLAGS:+ ${CPPFLAGS}}" + else + echo "::warning::Homebrew not found; relying on system LLVM/clang-tidy." + fi
302-315: Match run-clang-tidy version to clang-tidy and avoid duplicate -fix.
- Hardcoded 20.x may mismatch installed clang-tidy and cause option/diagnostic drift.
- You append -fix twice when run-clang-tidy is present.
- if [[ -x "$(command -v run-clang-tidy)" ]]; then - echo "Using run-clang-tidy from $(command -v run-clang-tidy)" - CLANG_TIDY=(run-clang-tidy -fix) - else - echo "Downloading run-clang-tidy script" - wget -O run-clang-tidy.py https://raw.githubusercontent.com/llvm/llvm-project/refs/heads/release/20.x/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py - CLANG_TIDY=(uv run --no-project --script -- run-clang-tidy.py) - fi - if [[ -x "$(command -v clang-apply-replacements)" ]]; then - echo "Using clang-apply-replacements from $(command -v clang-apply-replacements)" - CLANG_TIDY+=(-fix -clang-apply-replacements-binary="$(command -v clang-apply-replacements)") - else - echo "::warning::clang-apply-replacements not found in PATH, automatic fixing disabled." - fi + if [[ -x "$(command -v run-clang-tidy)" ]]; then + echo "Using run-clang-tidy from $(command -v run-clang-tidy)" + CLANG_TIDY=(run-clang-tidy) + else + echo "Downloading run-clang-tidy script" + LLVM_MAJOR="$(clang-tidy --version | sed -nE 's/.*version ([0-9]+).*/\1/p')" + LLVM_BRANCH="release/${LLVM_MAJOR}.x" + curl -fsSL -o run-clang-tidy.py "https://raw.githubusercontent.com/llvm/llvm-project/refs/heads/${LLVM_BRANCH}/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py" + CLANG_TIDY=(uv run --no-project --script -- run-clang-tidy.py) + fi + if [[ -x "$(command -v clang-apply-replacements)" ]]; then + echo "Using clang-apply-replacements from $(command -v clang-apply-replacements)" + CLANG_TIDY+=(-fix -clang-apply-replacements-binary="$(command -v clang-apply-replacements)") + else + echo "::warning::clang-apply-replacements not found in PATH, automatic fixing disabled." + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.0 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
322-324: Gatecmake --freshbehind a capability check.Older CMake releases (common on self-hosted runners) do not recognize
--fresh, so this step will fail before we obtaincompile_commands.json. Fall back torm -rf cmake-build && cmake …when--freshis unavailable.Suggested change:
- cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + if cmake --help 2>/dev/null | grep -q -- '--fresh'; then + cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + else + rm -rf cmake-build + cmake -S . -B cmake-build ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + fi
325-329: Only feed translation units to clang-tidy.The current
findincludes headers (*.h,*.hpp) that have no compile command;clang-tidyexits with “Error while processing…” and the workflow fails. Extract the translation units listed incompile_commands.json(e.g.,.c,.cc,.cpp,.cxx,.cu) instead of scanning headers.One option:
- CXX_FILES=$(find src -type f -iname "*.[ch]pp" -o -iname "*.cc" -o -iname "*.c" -o -iname "*.h") - "${CLANG_TIDY[@]}" -clang-tidy-binary="$(command -v clang-tidy)" \ - -p="cmake-build" ${CXX_FILES} || rc="$?" + mapfile -t CXX_FILES < <(python - <<'PY' +import json, os +with open("cmake-build/compile_commands.json") as fh: + seen = [] + exts = {".c", ".cc", ".cpp", ".cxx", ".cu"} + for entry in json.load(fh): + path = entry["file"] + if os.path.splitext(path)[1] in exts and path not in seen: + seen.append(path) +for path in seen: + print(path) +PY +) + if ((${#CXX_FILES[@]} == 0)); then + echo "::error::No translation units found in compile_commands.json" + exit 1 + fi + "${CLANG_TIDY[@]}" -clang-tidy-binary="$(command -v clang-tidy)" \ + -p="cmake-build" "${CXX_FILES[@]}" || rc="$?"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.0 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
XuehaiPan
force-pushed
the
retire-format-sh
branch
2 times, most recently
from
138457d to
23a43a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
323-323: Add fallback for older CMake when --fresh is unsupported.- cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + if cmake --help 2>/dev/null | grep -q -- '--fresh'; then + cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + else + rm -rf cmake-build + cmake -S . -B cmake-build ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + fiBased on past review comment.
325-325: Restrict clang-tidy inputs to translation units (exclude headers).Feeding headers that lack compile commands causes clang-tidy to error. Use only TU extensions.
- CXX_FILES=$(find src -type f -iname "*.[ch]pp" -o -iname "*.cc" -o -iname "*.c" -o -iname "*.h") + CXX_FILES=$(find src -type f \( -iname "*.c" -o -iname "*.cc" -o -iname "*.cpp" -o -iname "*.cxx" \))(Optional: extract from compile_commands.json for exact TUs.)
XuehaiPan
force-pushed
the
retire-format-sh
branch
3 times, most recently
from
1a76e51 to
743b42b
Compare
XuehaiPan
force-pushed
the
retire-format-sh
branch
from
743b42b to
dfd1c30
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.clang-tidy (1)
10-58: Resolve conflicting enable/disable checks to avoid ambiguity.You enable several checks and later disable the same ones. Order-dependent outcomes are brittle. Remove duplicates so intent is clear.
Apply:
# 1. Retained categories: easier to find bugs/performance issues clang-analyzer-*, - cppcoreguidelines-pro-type-static-cast-downcast, - cppcoreguidelines-pro-type-member-init, - cppcoreguidelines-pro-bounds-array-to-pointer-decay, - cppcoreguidelines-pro-bounds-pointer-arithmetic, cppcoreguidelines-slicing, - cppcoreguidelines-narrowing-conversions, performance-*,This keeps the explicit disables below authoritative and removes contradictions.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
313-313: Add fallback whencmake --freshis unavailable.Self-hosted runners may have CMake < 3.24;
--freshwill fail and break the job. Add a capability check and fallback.Apply:
- cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + if cmake --help 2>/dev/null | grep -q -- '--fresh'; then + cmake -S . -B cmake-build --fresh ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + else + rm -rf cmake-build + cmake -S . -B cmake-build ${CLANG_TIDY_CMAKE_OPTIONS} # no quotes here + fi
315-319: Don’t feed headers to clang-tidy; use compile_commands.json (prevents failures and ARG_MAX issues).Passing
*.h/*.hppcauses “Error while processing …” for headers without compile commands and risks long arg lists. Let run-clang-tidy enumerate TUs from-pinstead.Preferred:
- CXX_FILES=$(find src -type f -iname "*.[ch]pp" -o -iname "*.cc" -o -iname "*.c" -o -iname "*.h") - rc=0 - "${CLANG_TIDY[@]}" -clang-tidy-binary="$(command -v clang-tidy)" \ - -p="cmake-build" ${CXX_FILES} || rc="$?" + rc=0 + # Let run-clang-tidy traverse all translation units from compile_commands.json + "${CLANG_TIDY[@]}" -clang-tidy-binary="$(command -v clang-tidy)" -p="cmake-build" || rc="$?"If you must pass files, at least restrict to TUs:
- CXX_FILES=$(find src -type f -iname "*.[ch]pp" -o -iname "*.cc" -o -iname "*.c" -o -iname "*.h") + CXX_FILES=$(find src -type f \( -iname "*.c" -o -iname "*.cc" -o -iname "*.cpp" -o -iname "*.cxx" \))
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
320-323: Improve failure hint for local reproduction.Recommend the exact repro command users should run.
- echo "::error::clang-tidy found issues (exit code: ${rc}). Please run 'clang-tidy --fix' locally to fix them." + echo "::error::clang-tidy found issues (exit code: ${rc}). Reproduce locally with:" + echo " cmake -S . -B cmake-build -DCMAKE_EXPORT_COMPILE_COMMANDS=ON" + echo " run-clang-tidy -p cmake-build -fix -clang-apply-replacements-binary=\$(command -v clang-apply-replacements)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.clang-tidy(1 hunks).github/workflows/ci.yml(2 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)format.sh(0 hunks)requirements-lint.txt(1 hunks)src/layout/utils.cc(1 hunks)src/op/gemm.cc(1 hunks)src/op/parallel.h(1 hunks)src/runtime/runtime.cc(1 hunks)src/target/codegen_cuda.cc(1 hunks)src/target/codegen_webgpu.cc(1 hunks)src/target/cuda.h(1 hunks)src/tl_templates/cpp/half.hpp(2 hunks)src/tl_templates/cuda/common.h(1 hunks)src/transform/cluster_planning.cc(1 hunks)src/transform/common/loop_fusion_utils.h(1 hunks)src/transform/layout_inference.cc(3 hunks)src/transform/layout_reducer.cc(1 hunks)src/transform/lower_thread_allreduce.cc(4 hunks)src/transform/make_packed_api.cc(1 hunks)src/transform/storage_access.cc(1 hunks)src/transform/warp_specialized_rewriter.cc(4 hunks)
💤 Files with no reviewable changes (1)
- format.sh
✅ Files skipped from review due to trivial changes (4)
- src/transform/common/loop_fusion_utils.h
- src/layout/utils.cc
- src/op/parallel.h
- requirements-lint.txt
🚧 Files skipped from review as they are similar to previous changes (12)
- src/runtime/runtime.cc
- src/transform/layout_reducer.cc
- src/transform/cluster_planning.cc
- src/op/gemm.cc
- src/tl_templates/cpp/half.hpp
- src/transform/storage_access.cc
- src/target/codegen_webgpu.cc
- .pre-commit-config.yaml
- .gitignore
- src/transform/layout_inference.cc
- src/target/codegen_cuda.cc
- src/tl_templates/cuda/common.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.0 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
🔇 Additional comments (10)
src/target/cuda.h (1)
5026-5031: LGTM! Formatting improvements from clang-tidy.The spacing around the binary minus operator now aligns with standard C/C++ formatting conventions. These changes are purely stylistic with no functional impact.
src/transform/make_packed_api.cc (1)
480-480: LGTM: Efficient binding avoids copy.Binding
opt.value()as a const reference instead of copying is appropriate here, as theprim_funcis only read in the subsequent call toRequiresPackedAPI.src/transform/lower_thread_allreduce.cc (2)
499-501: LGTM: Preallocating and constructing in place.Reserving capacity upfront and using
emplace_backinstead ofpush_backare solid optimizations that avoid unnecessary reallocations and temporary object creation.
608-608: LGTM: Const reference bindings eliminate copies.Binding
shared_bufs[i]andlocal_bufs[i]as const references instead of by value avoids unnecessary Buffer copies. These variables are only read in subsequent operations, so const ref is both safe and efficient.Also applies to: 627-627, 643-643
src/transform/warp_specialized_rewriter.cc (4)
533-537: LGTM - Appropriate clang-tidy suppression.The ternary expression preserves the original semantics, and the
NOLINTNEXTLINEsuppression forperformance-move-const-argis appropriate here. The move is valid sinceblock_stmtis not const and won't be used after constructing theSeqStmt.
701-706: LGTM - Consistent pattern.This change follows the same pattern as line 533, correctly preserving semantics while addressing clang-tidy formatting requirements.
740-745: LGTM.Consistent application of the ternary pattern with appropriate suppression.
774-778: LGTM - Formatting update complete.Final instance of the consistent ternary pattern. All changes in this file are localized, preserve semantics, and appropriately integrate clang-tidy requirements.
.github/workflows/ci.yml (1)
297-310: Good: gate -fix behind clang-apply-replacements.Initialization no longer pre-seeds -fix and only adds it when the replacement tool exists. This avoids failures and duplication.
.clang-tidy (1)
2-7: Verify ExcludeHeaderFilterRegex support in CI clang-tidy
The sandbox lacked clang-tidy, so confirm your CI runner’s version recognizesExcludeHeaderFilterRegex. If not, replace with a compatibleHeaderFilterRegex:-ExcludeHeaderFilterRegex: '^(3rdparty|tvm)/.*$' +HeaderFilterRegex: '^(?!3rdparty/|tvm/).*$'
format.sh
Outdated
There was a problem hiding this comment.
would be better to maintain format.p referable, as it is more convenient though provides similar functionality to pre-commit. Developers should not be forced to install pre-commit when they're developing; instead, we could modify the content of format.sh to install and run pre-commit directly.
There was a problem hiding this comment.
How about moving format.sh as a Makefile with the following target (Unix-like systems always have make executable):
make formatWe can add more targets like:
make install
make docs
make test
make clang-tidy
make clean
...There was a problem hiding this comment.
I opened a new PR to resolve this.
XuehaiPan
force-pushed
the
retire-format-sh
branch
from
a28db27 to
30f6b6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
format.sh (1)
31-36: Uncommitted changes check is overly strict for local development.The script exits immediately if uncommitted changes are detected when no arguments are provided (lines 31–36). This prevents developers from running formatting on a dirty working tree, which is common during iterative development. Consider allowing formatting of staged files (via
git add) or adding a flag like--allow-dirtyto bypass this check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
format.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
🔇 Additional comments (2)
format.sh (2)
143-143: Auto-fix flag (-fix) may be unintended for local development.Line 143 runs
run-clang-tidywith the-fixflag, which modifies files in place. While suitable for auto-correcting during local development, this may be unintended if the intent is to report violations only. Clarify whether auto-fixing should occur, or if fixes should be reserved for a separate mode or CI context.If auto-fixing should only occur in a dedicated mode, consider:
- Adding a
--fixflag to the script to enable auto-fix behavior.- Removing
-fixfrom the defaultclang_tidy_changedcall.
59-75: Verify merge-base fallback strategy against repository setup.The
get_merge_basefunction attempts three strategies: upstream fetch,origin/main, and localmain. Ensure that the repository setup and typical branching workflow align with these assumptions. For instance, if pull requests are created against a fork withoutorigin/mainconfigured, the fallback to localmainmay not reflect the actual merge base.
XuehaiPan
force-pushed
the
retire-format-sh
branch
from
81e5ceb to
f969484
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
format.sh (1)
111-112: Comment contradicts condition logic.The comment "Check if clang-tidy is available" on line 111 misleads—the condition on line 112 (
if [[ ! -x "$(command -v clang-tidy)" ]]) checks if it's not available. Update the comment to clarify, e.g., "Install clang-tidy if missing" or "Check if clang-tidy is not available".
🧹 Nitpick comments (1)
format.sh (1)
124-158: Clang-tidy invocations lack error handling.The script invokes
run-clang-tidythree times (lines 125, 130, 143) without checking exit codes. If clang-tidy fails, the script continues silently. For local development, this may be acceptable, but consider adding error reporting or propagation, especially for line 143 which runs with-fixand modifies files.Optionally add error handling:
clang_tidy_files() { - run-clang-tidy -j 64 "$@" -p build + if ! run-clang-tidy -j 64 "$@" -p build; then + echo "Warning: clang-tidy check failed for specified files." >&2 + fi } clang_tidy_all() { - run-clang-tidy -j 64 src/*.cc -p build + if ! run-clang-tidy -j 64 src/*.cc -p build; then + echo "Warning: clang-tidy check failed on all files." >&2 + fi } clang_tidy_changed() { # ... existing logic ... if [[ -n "${CHANGED_FILES}" ]]; then echo "Running clang-tidy on changed files:" echo "${CHANGED_FILES}" CHANGED_FILES_SPACE="$(echo "${CHANGED_FILES}" | tr '\n' ' ')" - run-clang-tidy -j 64 ${CHANGED_FILES_SPACE} -p build -fix + if ! run-clang-tidy -j 64 ${CHANGED_FILES_SPACE} -p build -fix; then + echo "Warning: clang-tidy check failed on changed files." >&2 + fi else echo "No C/C++ files changed. Skipping clang-tidy." fi }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yaml(2 hunks)format.sh(2 hunks)requirements-lint.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements-lint.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
🔇 Additional comments (5)
.pre-commit-config.yaml (1)
35-35: Version bumps align well with tooling updates.Both
clang-format(v15.0.7 → v21.1.2) andruff(v0.14.0 → v0.14.1) are updated with clear comments indicating synchronization withrequirements-lint.txt. These versions support the CI and lint workflow improvements outlined in the PR.Also applies to: 44-44
format.sh (4)
18-21: Good defensive check for bash interpreter.The script now guards against running under non-bash shells, which prevents subtle failures from sh-specific limitations.
28-84: Mode-driven argument parsing is well-structured.The refactor cleanly separates
--all,--files, and default (changed-files-only) modes. The uncommitted-changes guard (lines 32-35) prevents accidental formatting of dirty working directories in the default mode. Theget_merge_base()fallback chain is robust.
86-94: Pre-commit initialization logic is correct.The inverted check (
if ! python3 -m pre_commit --version) correctly installs pre-commit only when it's missing. Hook initialization follows appropriately.
167-176: Post-format change detection is solid.The script correctly exits with failure if formatting operations leave uncommitted changes, prompting the developer to review and stage them before pushing.
2 participants
Summary by CodeRabbit
Chores
Bug Fixes
Style