perf: optimize use of `ClangParams` struct #231

2bndy5 · 2025-12-11T21:21:56Z

This clones a reference (pointer) to the same ClangParams instance instead of cloning the instance data (when processing each source file).

Some param types updated to reflect the actual life cycle of the ClangParams instance. Another param type change relies on internal mutability when passing around references to FileObj instances in an array.

Added auto-trait Debug to ClangVersions struct, in case debug printing is ever needed.

Removes a duplicate condition where ranges of changed lines are an empty list if lines-changed-only is Off.

Summary by CodeRabbit

Refactor
- Optimized memory handling in the analysis pipeline for improved efficiency.
- Refined line-filter logic for more consistent processing.
Tests
- Updated test variable naming for clarity.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

This clones a reference (pointer) to the same `ClangParams` instance instead of cloning the instance data (when processing each source file). Some param types updated to reflect the actual life cycle of the `ClangParams` instance. Another param type change relies on internal mutability when passing around references to `FileObj` instances in an array. Added auto-trait `Debug` to `ClangVersions` struct, in case debug printing is ever needed. Removes a duplicate condition where ranges of changed lines are an empty list if `lines-changed-only` is `Off`.

coderabbitai · 2025-12-11T21:24:41Z

Walkthrough

The pull request refactors ownership and borrowing patterns in the clang-tools module. Changes include removing mutability requirements from capture_clang_tools_output, updating the line-filter logic in run_clang_tidy to always compute ranges, and adding Debug derive to ClangVersions. All call sites are updated accordingly.

Changes

Cohort / File(s)	Summary
clang_tidy line-filter refactoring `cpp-linter/src/clang_tools/clang_tidy.rs`	Removed `LinesChangedOnly` import; line-filter logic now unconditionally computes `file.get_ranges` and applies filter only when ranges non-empty; test variable renames (`arc_ref` → `arc_file`)
clang_tools signature and ownership changes `cpp-linter/src/clang_tools/mod.rs`	`ClangVersions` now derives `Debug`; `capture_clang_tools_output` signature changed from `files: &mut Vec<...>` to `files: &[...]` and `clang_params: &mut ClangParams` to `clang_params: ClangParams` (moved by value); internal logic refactored to wrap clang_params in Arc and reuse across iterations
Call site adaptation `cpp-linter/src/run.rs`	Updated `capture_clang_tools_output` call in `run_main` to pass immutable slice reference and move clang_params by value instead of mutable borrow

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Extra attention areas:
- Verify Arc cloning and reuse of arc_params in the loop doesn't introduce unintended sharing or lifetime issues
- Confirm line-filter logic change in run_clang_tidy (always computing ranges vs. conditional behavior) is intentional and correct
- Validate that all callers of capture_clang_tools_output have been updated to match the new immutable slice and moved parameter pattern

Possibly related PRs

fix: clang-tidy diagnostic comments in PR review #77: Modifies capture_clang_tools_output implementation for control-flow fixes, directly impacting same function signature refactoring
refactor: switch to quick_xml library #101: Modifies run_clang_tidy function that receives line-filter logic changes in this PR
feat: add optional colored log output #52: Updates capture_clang_tools_output signature with parameter additions, overlapping the parameter ownership changes here

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main objective of the PR: optimizing ClangParams usage by cloning references instead of cloning the entire struct instance.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch tiny-optimization

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fde6ce5 and 5d6ad97.

📒 Files selected for processing (3)

cpp-linter/src/clang_tools/clang_tidy.rs (4 hunks)
cpp-linter/src/clang_tools/mod.rs (3 hunks)
cpp-linter/src/run.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (7)

📓 Common learnings

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 224
File: cpp-linter/src/clang_tools/clang_format.rs:103-139
Timestamp: 2025-12-11T20:19:05.142Z
Learning: In cpp-linter-rs, run_clang_format in cpp-linter/src/clang_tools/clang_format.rs must execute two clang-format commands sequentially when format_review is enabled: first to get formatted output, then to get XML replacements based on the original unformatted content. Concurrent execution would create a race condition because the commands operate on different file states. This sequential pattern is intentional and necessary given clang-format's capabilities.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.870Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 52
File: cpp-linter/src/clang_tools/mod.rs:146-146
Timestamp: 2024-10-02T07:55:08.948Z
Learning: In the `cpp-linter` project, the function `capture_clang_tools_output` is only called in `cpp-linter/src/run.rs`, and there are no other calls to this function elsewhere in the codebase.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.

📚 Learning: 2024-11-23T06:20:11.698Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/run.rs
cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2024-10-02T07:55:08.948Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 52
File: cpp-linter/src/clang_tools/mod.rs:146-146
Timestamp: 2024-10-02T07:55:08.948Z
Learning: In the `cpp-linter` project, the function `capture_clang_tools_output` is only called in `cpp-linter/src/run.rs`, and there are no other calls to this function elsewhere in the codebase.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/run.rs
cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2025-12-11T20:19:05.142Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 224
File: cpp-linter/src/clang_tools/clang_format.rs:103-139
Timestamp: 2025-12-11T20:19:05.142Z
Learning: In cpp-linter-rs, run_clang_format in cpp-linter/src/clang_tools/clang_format.rs must execute two clang-format commands sequentially when format_review is enabled: first to get formatted output, then to get XML replacements based on the original unformatted content. Concurrent execution would create a race condition because the commands operate on different file states. This sequential pattern is intentional and necessary given clang-format's capabilities.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/run.rs
cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2025-11-04T06:50:10.870Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.870Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/run.rs
cpp-linter/src/clang_tools/clang_tidy.rs

📚 Learning: 2024-11-23T14:10:31.760Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/clang_tools/mod.rs:149-159
Timestamp: 2024-11-23T14:10:31.760Z
Learning: Clang tools' `--version` output is consistent across all released versions, so adding extra error handling or unit tests for version extraction may be unnecessary. Tests are run after the clang tools are installed.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/run.rs

📚 Learning: 2025-01-21T09:56:32.771Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 101
File: cpp-linter/src/clang_tools/clang_format.rs:155-161
Timestamp: 2025-01-21T09:56:32.771Z
Learning: In cpp-linter-rs, the XML output being parsed is generated programmatically by clang-format tool. The only failure case for XML parsing is when clang-format produces a blank XML document, in which case falling back to empty results (using unwrap_or) is the desired behavior.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/clang_tools/clang_tidy.rs

🧬 Code graph analysis (1)

cpp-linter/src/clang_tools/clang_tidy.rs (2)

cpp-linter/src/common_fs/file_filter.rs (1)

new (17-24)

cpp-linter/src/common_fs/mod.rs (1)

new (44-53)

⏰ 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). (20)

GitHub Check: s390x-unknown-linux-gnu
GitHub Check: powerpc-unknown-linux-gnu
GitHub Check: aarch64-apple-darwin
GitHub Check: armv7-unknown-linux-gnueabihf
GitHub Check: arm-unknown-linux-gnueabi
GitHub Check: aarch64-unknown-linux-musl
GitHub Check: x86_64-unknown-linux-musl
GitHub Check: Build aarch64-unknown-linux-musl
GitHub Check: Build aarch64-unknown-linux-gnu
GitHub Check: Build armv7-unknown-linux-gnueabihf
GitHub Check: Build aarch64-apple-darwin
GitHub Check: Build x86_64-unknown-linux-gnu
GitHub Check: Build x86_64-unknown-linux-musl
GitHub Check: Build x86_64-apple-darwin
GitHub Check: Build x86_64-pc-windows-msvc
GitHub Check: Build i686-pc-windows-msvc
GitHub Check: Build current binary
GitHub Check: test (windows-latest)
GitHub Check: test (ubuntu-latest)
GitHub Check: Build FreeBSD

🔇 Additional comments (7)

cpp-linter/src/clang_tools/clang_tidy.rs (3)

19-22: LGTM!

The import cleanup is correct. LinesChangedOnly is now accessed through clang_params.lines_changed_only, so there's no need to import it directly in this file.

266-277: LGTM! Nice simplification of the line-filter logic.

The refactor removes a redundant condition by always computing ranges and applying the --line-filter argument only when ranges are non-empty. Since get_ranges() returns an empty list when lines_changed_only is Off, this achieves the same behavior with cleaner code.

430-446: LGTM!

The variable rename from arc_ref to arc_file improves clarity and consistency with naming conventions used elsewhere in the codebase.

cpp-linter/src/run.rs (1)

140-146: LGTM!

The call-site updates correctly align with the updated capture_clang_tools_output signature. Passing arc_files by immutable reference is safe since Arc<Mutex<FileObj>> provides interior mutability, and moving clang_params by value is appropriate since it's immediately wrapped in an Arc within the function.

cpp-linter/src/clang_tools/mod.rs (3)

188-188: LGTM!

Adding Debug derive to ClangVersions is a good practice for debugging and logging purposes without any runtime cost.

201-206: LGTM! Well-designed API improvement.

The signature changes are appropriate:

Using &[Arc<Mutex<FileObj>>] is more idiomatic than &mut Vec<...> since the function only iterates over files (mutation happens through the inner Mutex).

Taking ClangParams by value enables the efficient Arc wrapping pattern, avoiding per-file data clones.

243-248: LGTM! This is the key optimization of this PR.

Creating a single Arc<ClangParams> and cloning the Arc reference (cheap atomic reference count increment) instead of cloning the entire ClangParams struct data for each file is an efficient pattern for sharing read-only configuration across concurrent tasks.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2025-12-11T21:31:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.93%. Comparing base (fde6ce5) to head (5d6ad97).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   96.90%   96.93%   +0.03%     
==========================================
  Files          14       14              
  Lines        3034     3033       -1     
==========================================
  Hits         2940     2940              
+ Misses         94       93       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

2bndy5 added the enhancement New feature or request label Dec 11, 2025

2bndy5 mentioned this pull request Dec 11, 2025

feat: use tokio::Command to capture suggestions #224

Closed

2bndy5 merged commit 6e6e43d into main Dec 11, 2025
66 of 67 checks passed

2bndy5 deleted the tiny-optimization branch December 11, 2025 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

perf: optimize use of `ClangParams` struct #231

perf: optimize use of `ClangParams` struct #231

Uh oh!

2bndy5 commented Dec 11, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 11, 2025

Uh oh!

codecov bot commented Dec 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

perf: optimize use of ClangParams struct #231

perf: optimize use of ClangParams struct #231

Uh oh!

Conversation

2bndy5 commented Dec 11, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 11, 2025

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

perf: optimize use of `ClangParams` struct #231

perf: optimize use of `ClangParams` struct #231

2bndy5 commented Dec 11, 2025 •

edited by coderabbitai bot

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading