chore(lint): Rust C++ linter to dev profile + line-tables debug + RUST_BACKTRACE=full - #3335
Merged
Conversation
… debug + RUST_BACKTRACE=full The fastled-lint binary is disk-I/O bound (reads + parses source files in the lint hot path). Release-mode codegen does not measurably speed runtime up but costs ~10x cold-build wall time. Switch to the dev profile: - ci/lint_cpp_rs/Cargo.toml: add [profile.dev] with debug = "line-tables-only" so panic stack traces still resolve to file:line without dragging in full DWARF. - [profile.dev.package."*"] opt-level=1 for third-party crates. Cold build empirically halves vs opt-0-for-all because regex / rayon / walkdir have heavy generic instantiation that compiles slowly at opt-0; the opt-1 cost is paid once and cached across crate edits. - ci/lint_cpp/rust_binary_cache.py: drop --release from the build invocation; switch _binary_candidate_paths to look in target/.../debug/ instead of target/.../release/. - ci/lint_cpp/rust_bridge.py: set RUST_BACKTRACE=full when invoking the binary (setdefault so caller overrides win) so any panic produces a resolvable trace. The line-tables-only debuginfo makes the trace useful without the heavier compile cost of full debuginfo. - .claude/skills/new-cpp-lint/SKILL.md: update the documented invocation path from target/release/ to target/debug/. CI cache (ci/lint_cpp_rs/target) is keyed on Cargo.lock + src/**/*.rs; the profile switch does not break the existing cache key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR comprises five independent improvements: (1) switches ChangesDev Profile Build Switch
Lint Script Startup Optimization
Regex Pre-Compilation and Filesystem Caching Optimizations
Profiling Instrumentation for Linter Performance
Concurrent Linter Stage Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… env-gated profiling to fastled-lint `bash lint --cpp` was clocking 3m20s end-to-end. Profiling (added below) showed the fastled-lint Rust binary itself was 33s and the Python orchestrator on top was another ~25s. The remaining ~140s was uv's per-invocation project-sync probe firing inside the wrapper script. Wrapper fix: - `lint`: switch `uv run ci/lint.py "$@"` to `uv run --no-sync ci/lint.py "$@"`. The project uses hatchling (pure Python), there is no native extension to rebuild; the sync probe was pure overhead. Same escape that zackees/soldr#805 warns about. Manual `uv sync` is still required after pyproject.toml / uv.lock changes. Measured (cold cpp_lint cache, FastLED checkout): - Before: 3m20s end-to-end - After: 59s end-to-end - Saved: ~140s per invocation Profiling hooks (env-gated, no runtime cost when off): - ci/lint_cpp_rs/src/lint_core/processor_registry_cli.rs: - new `process_files_with_checkers_profiled()` wraps the existing method and emits `[PROFILE]` stderr lines for unique-paths, file read+parse (par), clone-from-cache, checker dispatch (par), sort, and the surrounding run_cli phases (create_checkers, collect_input_files, run_structural_passes, TOTAL). - existing `process_files_with_checkers()` is preserved as a thin forwarder so external callers (if any) keep working. - gated by FASTLED_LINT_PROFILE=1 env var. Profile output from a representative run (FastLED master): [PROFILE] create_checkers: 94.6us (78 checkers) [PROFILE] collect_input_files: 381ms (2730 files) [PROFILE] unique_paths build: 26ms (2730 unique) [PROFILE] file read+parse (par): 97ms (2730 files) [PROFILE] clone-from-cache: 100ms (2730 files) [PROFILE] checker dispatch (par): 30.9s (2730 files x 78 checkers) [PROFILE] sort violations: 600ns (0 violations) [PROFILE] run_structural_passes: 451ms (0 violations) [PROFILE] TOTAL run_cli: 32.0s Confirms the file-once, dispatch-to-N-checkers Grand Central Dispatch pattern is intact and rayon-parallel. The 30.9s checker dispatch is real linting work over 213k file/checker combos - the next round of optimization would target hot per-checker work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/lint_cpp_rs/src/lint_core/processor_registry_cli.rs`:
- Around line 500-502: The profile variable at line 501 uses ok().is_some()
which enables profiling whenever the FASTLED_LINT_PROFILE environment variable
exists, regardless of its value. Instead of just checking for presence, parse
the environment variable value explicitly to check if it equals "1" (or another
truthy value as per the documentation). Replace the is_some() check with a
condition that compares the actual string value from std::env::var to determine
if profiling should be enabled, ensuring that FASTLED_LINT_PROFILE=0 or empty
values correctly disable profiling.
🪄 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: 1f681ccd-f016-426e-bf50-71b4ec04298b
📒 Files selected for processing (2)
ci/lint_cpp_rs/src/lint_core/processor_registry_cli.rslint
✅ Files skipped from review due to trivial changes (1)
- lint
…sChecker (-3.5x rust binary)
Per-checker profiling exposed NoexceptSpecialMembersChecker burning
252 seconds of CPU across 946 src/fl files (~266ms/file). Next-slowest
checker was 20s. The hot path was in classify_noexcept_line:
for name in class_names {
let pattern = format!(r"\b{}\s*\(", regex::escape(name));
let Ok(regex) = Regex::new(&pattern) else { continue; }; // <-- !!!
for matched in regex.find_iter(&code) {
...
}
}
Called for every non-blank non-comment line of every file - so a header
with 30 classes and 500 lines paid for 15,000 Regex::new calls.
Regex::new is milliseconds per invocation; that's where the time went.
Fix: compile a single combined alternation regex per FILE, once:
let pattern = format!(r"\b({})\s*\(", names.iter().map(escape).join("|"));
let combined = Regex::new(&pattern)?;
Then iterate captures and look up the matched class name from
captures.get(1). One regex compile per file, not per line per name.
Plumbing:
- New ClassNameRegex struct in analysis_helpers.rs owns the names + the
combined regex; built once in NoexceptSpecialMembersChecker::check_file_content
before the line loop, then passed by reference into
classify_noexcept_line (which previously took the raw HashSet).
Per-checker profile, before/after:
before: 251,781ms 946 files NoexceptSpecialMembersChecker
after: 2,371ms 946 files NoexceptSpecialMembersChecker (106x)
Whole-binary profile:
before: TOTAL run_cli: 32.0s
after: TOTAL run_cli: 9.1s (3.5x)
End-to-end `bash lint --cpp`:
baseline: 3m 20s
+ --no-sync: 1m 11s
+ this fix: 52s
Also bundles in the per-checker-timing instrumentation from the
previous commit's diagnostic pass (gated by FASTLED_LINT_PROFILE=1,
zero runtime cost when off) so future regressions in any single
checker are immediately attributable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/lint_cpp_rs/src/lint_core/analysis_helpers.rs`:
- Around line 360-361: The code at line 360 silently discards regex compilation
errors by using .ok(), which converts the Result to an Option with no error
reporting. When the combined regex is None at line 394-395, the function returns
None without any logging or signal, causing all copy/move/default-constructor
detection to be silently skipped for that file. Instead of using .ok() to hide
the error, implement proper error handling by logging a warning when Regex::new
fails and provide a fallback approach (such as using a default/simplified regex
pattern or explicitly returning an error) so that regex compilation failures are
visible and don't silently disable constructor checks.
🪄 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: a6041972-1346-4020-a06b-5d3ccd5dd883
📒 Files selected for processing (3)
ci/lint_cpp_rs/src/checkers/test_structure.rsci/lint_cpp_rs/src/lint_core/analysis_helpers.rsci/lint_cpp_rs/src/lint_core/processor_registry_cli.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/lint_cpp_rs/src/lint_core/processor_registry_cli.rs
…peGlobalChecker (-2x rust binary)
Two more hot spots removed from the per-checker profile:
1. TestIncludePathsChecker (was 26s -> falls out of top 20)
all_test_header_filenames() did a WalkDir over the entire tests/
tree on every file (368 calls = 368 walks); top_level_headers()
read_dir'd per call too. Both are now memoized in
ci/lint_cpp_rs/src/lint_core/path_helpers.rs via OnceLock<Mutex<
HashMap>> keyed on (root_prefix, dir) - one walk per lint run.
2. CtypeGlobalChecker (was 5.8s -> falls out of top 20)
Three nested O(28) loops:
- whole-file pre-flight: 28 separate `content.contains(name)`
scans per file (28 * content_len byte comparisons each)
- per-line: 28 substring scans per non-comment line
- per-line: 28 separate find loops inside find_ctype_calls
Added ctype_any_function_regex() - one static OnceLock<Regex>
matching `\b(name1|name2|...)\b` for all 28 ctype + cstring names.
Single linear pass over content / line replaces 28-pass loop.
find_ctype_calls itself is unchanged because it builds a
(name, is_qualified) tuple list - the regex gate just prevents
it from being entered on lines that have no candidates at all.
Per-checker delta:
before TestIncludePathsChecker: 26,020ms 368 files
after TestIncludePathsChecker: <1300ms (out of top 20)
before CtypeGlobalChecker: 5,790ms 2382 files
after CtypeGlobalChecker: <1300ms (out of top 20)
Whole-binary delta vs the previous commit:
dispatch wall: 7.15s -> 3.84s (1.9x)
TOTAL run_cli: 9.11s -> 5.14s (1.8x)
End-to-end `bash lint --cpp`:
baseline: 3m 20s
+ --no-sync: 1m 11s
+ noexcept regex fix: 52s
+ this commit: 45s
Cumulative wins vs baseline: rust binary 6.3x, end-to-end 4.4x.
The remaining top spenders (MemberStyleChecker, SleepForChecker,
BannedMacrosChecker, BareAllocationChecker, AttributeChecker, ...)
are now clustered at 2.0-2.5s each across 2500+ files (<1ms/file),
which is normal per-line scanning work; no single dominator remains
inside the Rust binary. The remaining 40s in `bash lint --cpp`'s
cpp_linting bucket is the Python orchestrator + AST checks via
clang-tool-chain-query, which is the next profiling target.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d-to-end) Single largest remaining bottleneck after the Rust-side wins: - run_rust_linter: ~5s (Rust binary, all-cores rayon) - run_noexcept_ast_check: ~17s (single clang-query subprocess) - run_array_param_ast_check: ~10s (single clang-query subprocess) - run_checkers (Python): ~1-2s (mostly empty buckets today) All three heavy stages are independent and were running sequentially - 27+s of clang-query subprocess wall time gated behind Python's GIL. None of them mutate shared state during their work; merging happens after all three return. Fan out with a ThreadPoolExecutor: kick off the Rust pass and both clang-query passes on background threads, run the Python per-file pass on the foreground thread, then collect futures and merge. GIL is a non-issue because each background stage spends ~all of its time inside subprocess.run waiting on subprocess I/O - the GIL is released for the wait, so the threads truly overlap. Wall time collapses from sum(stages) to max(stages): before: 5 + 17 + 10 + 2 = 34s sequential after: max(17, 10, 5, 2) = ~17s Measured end-to-end `bash lint --cpp`: before this commit: 45s (cpp_linting bucket: 40s) after this commit: 23s (cpp_linting bucket: 18s) Cumulative wins vs original baseline: baseline: 3m 20s + --no-sync: 1m 11s + noexcept-regex fix: 52s + ctype+test-walk cache fix: 45s + parallel AST fan-out: 23s ------------------------------------------ Speedup: 8.7x run_checkers() stays on the foreground thread - most of its scope buckets are empty today (the Rust binary owns the per-file dispatch), so it returns in <2s and there's no benefit to backgrounding it. If a heavy Python checker ever returns it can move into the pool trivially. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/lint_cpp_rs/src/lint_core/path_helpers.rs (1)
577-583: ⚡ Quick winReduce cache type complexity with local type aliases.
These nested cache types are hard to parse and maintain; adding aliases will keep signatures readable without changing behavior.
♻️ Suggested cleanup
+type HeaderSet = HashSet<String>; +type SharedHeaderSet = std::sync::Arc<HeaderSet>; +type TopLevelHeadersCacheMap = HashMap<(String, String), SharedHeaderSet>; +type TestHeaderFilenamesCacheMap = HashMap<String, SharedHeaderSet>; + fn top_level_headers_cache( -) -> &'static std::sync::Mutex<HashMap<(String, String), std::sync::Arc<HashSet<String>>>> { +) -> &'static std::sync::Mutex<TopLevelHeadersCacheMap> { static CACHE: std::sync::OnceLock< - std::sync::Mutex<HashMap<(String, String), std::sync::Arc<HashSet<String>>>>, + std::sync::Mutex<TopLevelHeadersCacheMap>, > = std::sync::OnceLock::new(); CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new())) } @@ fn all_test_header_filenames_cache( -) -> &'static std::sync::Mutex<HashMap<String, std::sync::Arc<HashSet<String>>>> { +) -> &'static std::sync::Mutex<TestHeaderFilenamesCacheMap> { static CACHE: std::sync::OnceLock< - std::sync::Mutex<HashMap<String, std::sync::Arc<HashSet<String>>>>, + std::sync::Mutex<TestHeaderFilenamesCacheMap>, > = std::sync::OnceLock::new(); CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new())) }Also applies to: 620-624
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/lint_cpp_rs/src/lint_core/path_helpers.rs` around lines 577 - 583, The top_level_headers_cache function contains deeply nested generic types that are difficult to read and maintain. Define type aliases for the complex types before the function definition, specifically for the HashMap<(String, String), std::sync::Arc<HashSet<String>>> and its Mutex wrapper, then use these aliases in the function return type and the static CACHE variable declaration to simplify the code without changing any behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ci/lint_cpp_rs/src/lint_core/path_helpers.rs`:
- Around line 577-583: The top_level_headers_cache function contains deeply
nested generic types that are difficult to read and maintain. Define type
aliases for the complex types before the function definition, specifically for
the HashMap<(String, String), std::sync::Arc<HashSet<String>>> and its Mutex
wrapper, then use these aliases in the function return type and the static CACHE
variable declaration to simplify the code without changing any behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63aa488b-f099-44ad-b98b-49651faee180
📒 Files selected for processing (2)
ci/lint_cpp_rs/src/checkers/types_and_tests.rsci/lint_cpp_rs/src/lint_core/path_helpers.rs
… runs) The two clang-query AST passes (noexcept + decayed-array) cost 17s and 10s respectively even when no source has changed - clang-query re-parses the tree from scratch every invocation. Their output is fully determined by: - source files in the requested scope (src/fl, platforms, third_party) - the baseline file (lists known-existing violations to subtract) - the tool's own Python source (so rule changes invalidate) Added ci/lint_cpp/ast_cache.py: blake2b fingerprint over (path, size, mtime_ns) of every input file, stored at .cache/ast_lint/<name>.fingerprint, paired with a JSON sidecar .cache/ast_lint/<name>.violations.json that replays the last run's violations verbatim on a cache hit. ~50ms to compute the fingerprint over ~2000 files; ~10ms to deserialize the JSON. atomic write via tmp+replace so a concurrent invocation cannot observe a torn pair. Cache key intentionally covers violations themselves, not just zero-violation runs - if you don't fix the violation you see the same output instantly on the next run; if you do fix it the underlying file changes and the cache invalidates correctly. Single-file mode (`run_*_ast_check(file_path=...)`) bypasses the cache because the fingerprint cost over the whole scope would dwarf the per-file check. Wiring: run_noexcept_ast_check + run_array_param_ast_check now extract their actual work into a local `_run()` closure and route through cached_ast_check(name, scope, tool_sources, baseline_path, runner=_run). The closure preserves the existing NoexceptCheckError / ArrayParamCheckError fallback (skips the ratchet with a stderr warning when clang-query is missing). Measured `bash lint --cpp`: baseline: 3m 20s + --no-sync: 1m 11s + noexcept regex fix: 52s + ctype+test-walk cache fix: 45s + parallel AST fan-out: 23s + this commit (cold): 25s (no-op vs previous) + this commit (warm): 4s ← new steady state Cumulative speedup vs baseline: 8x cold, 50x warm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/lint_cpp/run_all_checkers.py (1)
778-778: ⚡ Quick winMove
ThreadPoolExecutorto the module import block.Line 778 introduces a standard-library import inside
main(). Keep it with the other top-level stdlib imports to satisfy the repository import-order rule.Proposed fix
+# Add with the other top-level standard-library imports: +from concurrent.futures import ThreadPoolExecutor + ... - from concurrent.futures import ThreadPoolExecutor - with ThreadPoolExecutor(max_workers=3) as pool:As per coding guidelines, "
**/*.py: All system imports must be at the top of the file in proper order: standard library, third-party, local."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/lint_cpp/run_all_checkers.py` at line 778, The import of ThreadPoolExecutor from concurrent.futures is currently inside the main() function instead of at the module level. Move this import statement to the top of the file with the other standard library imports to comply with the repository's import-order guidelines which require all standard library imports to be grouped together at the beginning of the file before any other imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ci/lint_cpp/run_all_checkers.py`:
- Line 778: The import of ThreadPoolExecutor from concurrent.futures is
currently inside the main() function instead of at the module level. Move this
import statement to the top of the file with the other standard library imports
to comply with the repository's import-order guidelines which require all
standard library imports to be grouped together at the beginning of the file
before any other imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0777c1ab-4173-40af-b726-d03592ddbdbf
📒 Files selected for processing (1)
ci/lint_cpp/run_all_checkers.py
Same anti-pattern as the previous round: each checker walked every line of every applicable file even when the file plainly contains nothing the checker is looking for. Per-line bail-outs at the bottom of the loop body still fired, but only after running the comment-state machine and allocating split_line_comment substrings on every line. Added whole-file `file_content.content.contains(...)` early-exits to: - SleepForChecker - bail unless file contains "sleep_for" - BannedMacrosChecker - bail unless file contains "__has_include" or "static_assert" (the only two macros the body actually checks for - the struct name is broader than its current rule set) - BareAllocationChecker - bail unless file contains any of new / delete / malloc / calloc / realloc / free - NumericLimitMacroChecker - bail unless file contains "_MAX" or "_MIN" - WeakAttributeChecker - bail unless file contains "weak" - BannedDefineChecker - bail unless file contains "#if " (with space, to skip pure `#ifdef`/`#ifndef` files which the body would never flag) The win is per-file but compounds at scale: each removed checker frees its rayon-thread slice to do something useful, and removes the per-line allocations that were happening on the never-violates path. Per-checker profile shows SleepFor / NumericLimit / WeakAttribute / BannedDefine all fall out of the top 20. The remaining checkers are clustered at 1.3-2.7s each across 2500+ files; rayon parallelism is keeping the wall-clock dispatch flat as work shifts between threads. Wall-time delta: before this commit (cold): 25s after this commit (cold): 13s (AST cache hit, Rust + glue only) warm steady state: 4s (unchanged) The 25s number was actually misleading - it included the first cold AST-cache write. The real "C++ changed but not in src/fl AST scope" case is the 13s the dev sees on every header edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… binary)
The dispatcher's clone-from-cache phase was deep-cloning a String of
file content + Vec<String> of lines per file every invocation,
costing ~140ms of wall time on a 2730-file run (~50us per file).
Change file_cache from HashMap<PathBuf, FileContent> to
HashMap<PathBuf, Arc<FileContent>>. The collect-into-Vec step now
calls Arc::clone (refcount bump, ~tens of nanoseconds per file)
instead of a deep clone, and the rayon-parallel dispatch loop holds
&Arc<FileContent> through Rust's auto-deref so the per-checker code
is unchanged.
Profile delta on a 2730-file run:
before after
file read+parse (par) 121ms 90ms -25%
clone-from-cache 142ms 6ms -95%
checker dispatch (par) 3.98s 3.54s -11%
TOTAL run_cli 5.6s 4.5s -20%
End-to-end `bash lint --cpp`:
cold (AST cache hit): 13s -> 11s
warm steady-state: 4s -> 3s
Cumulative numbers vs the original 3m 20s baseline:
cold: 3m 20s -> 11s (18x)
warm: 3m 20s -> 3s (67x)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nsics) - IncludePathsChecker: bail unless file contains "#include" anywhere. Most src/fl files do have at least one, so the saving is bounded, but it cleanly short-circuits the rare empty-stub case. - SimdIntrinsicsChecker: added simd_any_pattern_regex() - a static OnceLock<Regex> built once from the 27-entry SIMD_PATTERNS table. Most files have ZERO SIMD content; the per-line walk was doing 27 substring searches per non-comment line for nothing. Single regex pre-flight skips the walk entirely on those files. Checker fell out of top 20. Profile delta: dispatch wall: 3.54s -> 3.32s TOTAL run_cli: 4.50s -> 4.40s The remaining ~3.3s dispatch is line-by-line work in 14 checkers each processing 2000-2700 files at ~1ms/file - rayon is keeping every core busy and per-checker work is mostly comment-mask + regex match cost that can't be early-exited cheaply. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
…k (-28% truly-cold) find_missing_noexcept(scope=\"all\") and find_decayed_array_params(scope=\"all\") both dispatched 3 independent clang-query subprocesses (platforms / fl / third_party) sequentially in a Python for loop. Each TU is a fully self-contained parse - no shared state between them. Wrap the for loop in a ThreadPoolExecutor with max_workers=len(tus). clang-query subprocess.run releases the GIL during the I/O wait, so the three subprocesses really do execute in parallel. Single-TU scopes (when called for a single file under a specific subpath) keep the direct path with no executor overhead. Wall-time delta on a truly-cold run (AST cache wiped + src/fl edit): before: 25s after: 18s -28% Cumulative speedup vs the original 3m 20s baseline: warm: 3-5s (40-67x, unchanged) cold-edit: 11s (18x, unchanged) truly cold: 25s -> 18s (11x, was 8x) The two outer parallel layers compose: - run_all_checkers.py fans out rust binary + 2 AST checks - each AST check fans out 3 clang-query subprocesses Total live process count peaks at ~7 (rust + 6 clang-query). On a 16-core dev box they don't compete; on smaller CI runners they may serialize a little but the win is still net positive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cap on submitted-future count means this is a no-op for the current 3-TU per-AST-check and 3-stage outer fan-out, but makes future TU splitting drop straight in without re-touching the constants.
…wall)
The 3-TU bundling (platforms / fl / third_party) capped clang-query
parallelism at 3, which limited the AST passes to ~17s + ~10s wall on
a cold run. The fl/ TU alone was ~12s of that.
The repo already ships per-subdir TU shims at src/fl/build/fl.*+.cpp
for the existing unity build - 23 of them, one per fl/<subdir>, each
a 6-line shim that includes platforms/new.h + fl/system/arduino.h +
fl/<subdir>/_build.cpp.hpp. Same shape as our hand-rolled TU under
ci/tools/, just at finer granularity.
Switch find_missing_noexcept and find_decayed_array_params to
enumerate src/fl/build/fl.*+.cpp at runtime and submit one
clang-query future per shim to the existing ThreadPoolExecutor.
Per-TU file_regex is narrowed to .*src.fl.<subdir_dotted>.* so hits
only fire for files actually parsed by that shim.
"all" scope expansion is now ~25 work units (platforms + 23 fl
shims + third_party). With the cpu_count() default on max_workers
from the previous commit, all 25 run concurrently on a 16-core box;
smaller hosts cap at host cpu count and serialize the excess.
fl.system.sd+.cpp's compound `system.sd` naming flows through the
same `name[3:-len("+.cpp")]` parser.
Legacy ci/tools/_noexcept_check_*_tu.cpp files stay as fallback
(returned by _fl_subdir_tus when src/fl/build is missing) so the
checkers continue to work in checkouts that predate the build/ tree.
Measured deltas:
noexcept pass wall: 17.0s -> 7.9s (2.2x)
array-param pass wall: 10.0s -> 7.0s (1.4x)
end-to-end truly cold: 25s -> 16s (1.6x)
End-to-end win is bounded by max(noexcept, array-param) because the
run_all_checkers fan-out already runs them in parallel. Further gains
would need to consolidate to a single clang-query pass that runs both
matchers against the same parsed AST.
Cumulative speedup vs the original 3m 20s baseline:
warm: 3-5s (40-67x)
cold-edit: 11s (18x)
truly cold: 25s -> 16s (12.5x, was 11x)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ery session per TU Both AST passes were independently dispatching clang-query subprocesses per TU - the 25-TU \"all\" scope spawned 50 clang-query processes total (25 noexcept + 25 array-param), each one re-parsing the same TU independently. Parse dominates clang-query wall time; running both matchers against the same parsed AST halves the per-TU CPU work. New module ci/tools/check_ast_combined.py exposes find_combined_hits(scope) which submits one clang-query session per TU with BOTH matchers in the script body (different .bind() names so the diag output can be routed back to the right hit constructor). Reuses the original _find_array_params_in_signature and _signature_is_exempt filters from check_array_params - my first cut wrote simplified replacements and surfaced ~6k false-positive array-param hits before I caught it. run_all_checkers.py now fans out 2 stages instead of 3 (rust binary + combined AST instead of rust + noexcept + array-param). Per-check AST caches (.cache/ast_lint/noexcept_ast.* and array_param_ast.*) still wrap the combined call so a warm run hits both caches and skips clang-query entirely. The combined runner uses a local dict to share the single _shape() invocation between the two cached_ast_check calls - the first cache miss runs clang-query once, the second cache miss reads from the dict. Drive-by: the wider AST scope from the previous commit (per-subdir fl.*+.cpp TUs) revealed that the existing _TU_FL hand-rolled shim was NOT pulling in fl/system/sd/ via the unity build (linker tree-shake design - SD chain lives in its own +.cpp.o). The new TU enumeration correctly includes fl.system.sd+.cpp which surfaced a real missing FL_NO_EXCEPT on FileSystem::beginSd. Added FL_NO_EXCEPT to both declaration (file_system.h) and definition (file_system_sd.cpp.hpp) so the noexcept ratchet passes. Wall-time delta (truly cold, AST cache wiped + src/fl edit): before: 16s after: 15s Modest win because noexcept + array-param were already running in parallel at the outer fan-out level; the combined refactor cuts CPU work (and peak clang-query process count, ~50 -> ~25) but the wall gain is bounded by max(noexcept_wall, array_param_wall) ≈ noexcept's solo cost. Cumulative speedup vs original 3m 20s baseline: warm: 4s (50x) cold-edit: ~11s (18x) truly cold: 15s (13x) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ive header edits) The Python fingerprint cache for the clang-query AST passes was narrow-scoped: for the \"all\" scope it walked only src/fl + src/platforms + src/third_party. But clang-query parses each TU's TRANSITIVE include closure, which pulls in headers from src/ root (FastLED.h, crgb.h, chipsets.h, ...) and src/lib8tion/, src/extras/, etc. Concrete failure mode: edit src/crgb.h → noexcept_ast.fingerprint does not change → cache hit returns stale violations → user sees \"no new violations\" output that does not reflect the actual current state of the AST. Silent correctness bug. Fix: walk the full src/ tree regardless of scope. Cost is ~100 extra files in the fingerprint hash (~2000 -> ~2100), totalling about an extra 5ms of stat calls. The scope argument is retained in the function signature so the cache file naming stays per-scope, but no longer narrows which files are hashed. Verified by touching src/FastLED.h: the cache correctly invalidates on both the touch and the revert, where before the touch would have been silently absorbed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- FASTLED_LINT_PROFILE env var now requires explicit truthy value
('1'/'true'/'yes'/'on') instead of any-non-empty via .ok().is_some().
Was enabling profiling for any value including empty string.
- ClassNameRegex::build now logs a stderr warning when regex compilation
fails instead of silently disabling constructor noexcept detection.
- Drop bogus #include "fl/stl/make_shared.h" in fled_filesystem.cpp —
make_shared is provided by shared_ptr.h (already included on next line).
This was breaking the unit-test build on linux/macos/windows.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
On cold CI runners (notably macOS) where soldr's GitHub-API release lookup for the zccache binary hits the per-IP rate limit (HTTP 403), soldr falls back to cargo install zccache which compiles from source. That can take 4-6 minutes — exceeding the prior 300s lint-runner timeout and failing the job spuriously. Bumping to 900s gives the cargo install fallback enough headroom; still well under the workflow's 30-minute job timeout-minutes cap. Follow-up work in zccache itself (manifest.json-based release lookup with fuzzy target-triple match) will eliminate the fallback path entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s mentioning cargo don't false-positive
The hook scans Bash commands for bare cargo/rustc/rustfmt/clippy-driver
invocations. Prior version matched the literal word inside heredoc bodies
too, so a commit like:
git commit -m "$(cat <<'EOF'
fix: bump timeout because cargo install zccache is slow
EOF
)"
would falsely block on the word `cargo` appearing in the commit-message
body. The hook is supposed to detect *invocations*, not text content.
Add strip_heredoc_bodies(command) that replaces heredoc body content
with an empty body before regex scanning. Preserves the opener line
(so `cat <<EOF` still looks like a real command position) and the
closing delimiter (so post-heredoc command boundaries stay correct).
Validated against 7 synthetic cases:
- bare cargo build → blocked (correct)
- soldr cargo build → allowed (correct)
- cd && cargo build → blocked (correct)
- cd && soldr cargo build → allowed (correct)
- heredoc with cargo in body → allowed (was the false positive)
- real cargo invocation + heredoc body mentioning cargo → blocked (correct)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The fastled-lint binary is disk-I/O bound (reads + parses source files in the lint hot path). Release-mode codegen does not measurably speed runtime up but costs ~10x cold-build wall time. Switch to the dev profile.
Changes
ci/lint_cpp_rs/Cargo.toml— add[profile.dev]withdebug = \"line-tables-only\"so panic stack traces still resolve to file:line without dragging in full DWARF.[profile.dev.package.\"*\"]opt-level=1for third-party crates (empirically halves cold build vs opt-0-for-all; heavy generics inregex/rayon/walkdircompile very slowly at opt-0).ci/lint_cpp/rust_binary_cache.py— drop--releasefrom the build invocation; switch_binary_candidate_pathsto look intarget/.../debug/instead oftarget/.../release/.ci/lint_cpp/rust_bridge.py— setRUST_BACKTRACE=fullwhen invoking the binary (setdefaultso caller overrides win). Paired with the line-tables-only debuginfo, any panic produces a resolvable file:line trace..claude/skills/new-cpp-lint/SKILL.md— update the documented direct-invocation path fromtarget/release/totarget/debug/.Why
RUST_BACKTRACE=fullTest plan
bash lint --cpp— green_binary_candidate_pathscorrectly resolvestarget/x86_64-pc-windows-msvc/debug/fastled-lint.exe🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
FASTLED_LINT_PROFILE.Chores
pyproject.toml/uv.lockchanges).