feat: use tokio::Command to capture suggestions
#224
Conversation
WalkthroughThis PR converts clang tool execution from synchronous to asynchronous operations using tokio::process::Command. The tokio dependency is updated to include the "process" feature. Key functions (run_clang_format, run_clang_tidy, analyze_single_file, capture_version) are converted to async with signatures updated to use Arc<Mutex> instead of MutexGuard references. The orchestration module is refactored to coordinate async tasks and updated public APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (1)
cpp-linter/src/clang_tools/mod.rs (1)
245-260: Consider simplifying the spawn pattern to avoid the double-await.The current code spawns a future that returns another future, requiring
output?.await?. This works but could be simplified.Apply this diff to call
.awaitinside the spawned task:for file in files { let arc_params = Arc::new(clang_params.clone()); let arc_file = Arc::clone(file); - executors.spawn(async move { analyze_single_file(arc_file, arc_params) }); + executors.spawn(async move { analyze_single_file(arc_file, arc_params).await }); } while let Some(output) = executors.join_next().await { - let (file_name, logs) = output?.await?; + let (file_name, logs) = output??;Alternatively, since
analyze_single_fileis already anasync fn, you could spawn it directly:executors.spawn(analyze_single_file(arc_file, arc_params));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/clang_tools/clang_format.rs(6 hunks)cpp-linter/src/clang_tools/clang_tidy.rs(6 hunks)cpp-linter/src/clang_tools/mod.rs(4 hunks)cpp-linter/src/run.rs(2 hunks)cpp-linter/tests/comments.rs(1 hunks)cpp-linter/tests/paginated_changed_files.rs(2 hunks)cpp-linter/tests/reviews.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/tests/reviews.rscpp-linter/tests/paginated_changed_files.rscpp-linter/src/run.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/clang_tools/clang_format.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/tests/reviews.rscpp-linter/tests/paginated_changed_files.rscpp-linter/src/run.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rs
📚 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/tests/reviews.rscpp-linter/tests/paginated_changed_files.rscpp-linter/src/run.rscpp-linter/tests/comments.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.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/run.rscpp-linter/src/clang_tools/mod.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/run.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/clang_tools/clang_format.rs
🧬 Code graph analysis (5)
cpp-linter/tests/reviews.rs (1)
cpp-linter/tests/common/mod.rs (1)
mock_server(82-84)
cpp-linter/tests/paginated_changed_files.rs (1)
cpp-linter/src/rest_api/github/specific_api.rs (1)
new(33-80)
cpp-linter/tests/comments.rs (2)
cpp-linter/src/main.rs (1)
env(11-11)cpp-linter/tests/common/mod.rs (1)
mock_server(82-84)
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)
cpp-linter/src/clang_tools/clang_format.rs (1)
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: build-rustdoc
- GitHub Check: x86_64-apple-darwin
- GitHub Check: aarch64-apple-darwin
- GitHub Check: x86_64-unknown-linux-musl
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: test (windows-latest)
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-apple-darwin
- GitHub Check: Build aarch64-pc-windows-msvc
- GitHub Check: Build x86_64-unknown-linux-gnu
- GitHub Check: Build aarch64-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 FreeBSD
- GitHub Check: build-py-binding
- GitHub Check: Build current binary
🔇 Additional comments (36)
cpp-linter/Cargo.toml (1)
31-31: LGTM!The addition of
processandfsfeatures to tokio is appropriate for the async refactor. These enabletokio::process::Commandandtokio::fsoperations used inclang_tidy.rsandclang_format.rs.cpp-linter/tests/paginated_changed_files.rs (3)
40-53: LGTM!The
unsafeblocks for environment variable mutations align with newer Rust lint rules that flagenv::set_varas potentially unsafe due to thread safety concerns in multi-threaded test environments. This is a consistent pattern applied across the test suite.
57-66: LGTM!Consistent with the pattern for wrapping
env::set_varin unsafe blocks.
80-82: LGTM!Consistent unsafe wrapping for environment variable setup.
cpp-linter/tests/reviews.rs (3)
75-85: LGTM!Consistent
unsafeblock usage for environment variable mutations in test setup.
90-92: LGTM!Consistent pattern for
GITHUB_EVENT_PATHsetup.
98-100: LGTM!Consistent pattern for
GITHUB_API_URLsetup.cpp-linter/tests/comments.rs (3)
65-75: LGTM!Consistent
unsafeblock usage for environment variable setup in test code.
81-83: LGTM!Consistent pattern for conditional
GITHUB_EVENT_PATHsetup.
90-92: LGTM!Consistent pattern for
GITHUB_API_URLsetup.cpp-linter/src/run.rs (7)
165-177: LGTM!The
unsafeblock forenv::remove_varand the switch to.unwrap()for successful test cases is appropriate for test code.
182-187: LGTM!Consistent pattern applied to
version_commandtest.
192-203: LGTM!Consistent pattern applied to
force_debug_outputtest.
208-218: LGTM!Consistent pattern applied to
no_version_inputtest.
222-234: LGTM!The
pre_commit_envtest correctly places bothenv::remove_varandenv::set_varinside theunsafeblock, and appropriately usesassert!(result.is_err())since this test expects failure.
240-253: LGTM!Consistent pattern applied to
no_analysistest.
257-267: LGTM!The
bad_repo_roottest correctly usesassert!(result.is_err())for expected failure scenario.cpp-linter/src/clang_tools/clang_tidy.rs (5)
251-286: LGTM!The async signature change and the pattern of extracting
file_nameand buildingargswhile holding the lock briefly, then releasing it before async I/O operations, is the correct approach. This avoids holdingMutexGuardacross.awaitpoints which would preventSendrequirements.
287-300: LGTM!Using
tokio::fs::read_to_stringfor async file reading is appropriate for the async context.
301-317: LGTM!The command construction and async execution using
tokio::process::Commandwith.output().awaitis correctly implemented.
334-352: LGTM!The pattern of reading patched content, restoring original content using async I/O, and then acquiring the lock again to update
tidy_adviceis well-structured. The lock is held only for the brief mutation offile.tidy_advice.
435-483: LGTM!Test properly updated to async with
Arc<Mutex<FileObj>>pattern matching the new function signature.cpp-linter/src/clang_tools/mod.rs (4)
138-148: LGTM!The pattern of acquiring the lock briefly to extract
file_name, then releasing it before async operations, correctly avoids holdingMutexGuardacross await points.
149-187: LGTM!Using the extracted
file_namefor filter checks and logging avoids repeated lock acquisitions during the async operations.
191-191: LGTM!Deriving
DefaultforClangVersionsis a clean way to initialize the struct withNonevalues.
204-205: LGTM!Taking an immutable reference
&Vec<Arc<Mutex<FileObj>>>is appropriate since the function only needs to cloneArcs for spawning tasks.cpp-linter/src/clang_tools/clang_format.rs (10)
18-21: LGTM! Default handling correctly addresses XML parsing edge cases.The
Defaultderive and#[serde(default)]attribute work together to handle two scenarios:
- When clang-format produces empty stdout,
FormatAdvice::default()is used (line 154)- When XML has no
<replacement>elements, thedefaultattribute provides an empty Vec instead of failing deserializationThis aligns with the learning that valid XML with no replacements should parse successfully.
81-84: LGTM! Function signature correctly adapted for async execution.The signature change from
&mut FileObjto&Arc<Mutex<FileObj>>enables thread-safe shared access required by the async runtime, and theasync fnreturn type supports non-blocking command execution.
87-99: LGTM! Excellent mutex usage pattern.The code locks the mutex only to extract necessary data (file_name, ranges), then releases it before awaiting I/O operations. This minimizes lock contention and prevents holding the lock across async suspension points.
100-121: LGTM! Async command execution properly implemented.The optional format fixes retrieval correctly uses
tokio::process::Commandwith.await, includes clear logging, and provides helpful error context.
122-136: LGTM! Second command execution correctly structured.The code properly constructs a separate command instance with modified arguments to retrieve XML replacements, and includes appropriate error context.
146-156: LGTM! Robust default handling for empty output.The explicit check for empty stdout before parsing avoids errors when clang-format produces no output, falling back to
FormatAdvice::default()as intended. Based on learnings, this correctly handles the case where clang-format produces blank output.
158-162: LGTM! Async file reading correctly implemented.The
tokio::fs::readcall properly awaits file I/O, and the file path (derived fromfile.name.to_string_lossy()) is consistent with what's passed to clang-format commands.
181-186: LGTM! Second mutex lock properly scoped.The function locks the mutex a second time only to write the final result, after all async I/O has completed. The two lock scopes are separate and non-overlapping, avoiding deadlocks while maintaining thread safety.
201-215: LGTM! Test validates the serde default behavior.This test confirms that XML with a valid structure but no
<replacement>elements parses successfully, which is essential for the#[serde(default)]attribute added on line 21. This complements the existingparse_blank_xmltest and addresses the PR objective of properly parsing XML with no replacements.
4-9: Imports are correctly configured for async conversion.The imports properly support the async refactoring:
Arc<Mutex<>>for thread-safe sharing,anyhowfor error context, and tokio'sfsandprocess::Commandfor async I/O operations. The required tokio features (processandfs) are already enabled incpp-linter/Cargo.toml.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cpp-linter/src/clang_tools/mod.rs (3)
138-188: Async per-file analysis looks sound; minor cleanups possibleThe async refactor here looks correct: the mutex is only held while copying
FileObj.name, so no lock is held acrossawait, and poisoned locks are handled explicitly.Two small polish suggestions:
- You can avoid the extra allocation at the end by moving
file_nameinstead of cloning it:- Ok((file_name.clone(), logs)) + Ok((file_name, logs))
- The filter checks can be simplified to avoid the duplicated
is_some_and(..) || is_none()pattern, e.g.:- if clang_params - .format_filter - .as_ref() - .is_some_and(|f| f.is_source_or_ignored(file_name.as_path())) - || clang_params.format_filter.is_none() - { + if clang_params + .format_filter + .as_ref() + .map_or(true, |f| f.is_source_or_ignored(file_name.as_path())) + {and similarly for
tidy_filter. Behavior is unchanged; it’s just more idiomatic.
191-198: Consider also derivingDebugforClangVersionsNow that
ClangVersionsderivesDefault, you might also wantDebugfor easier logging and diagnostics, especially around tool version discovery:-#[derive(Default)] +#[derive(Default, Debug)] pub struct ClangVersions {If
Debugwas intentionally dropped (e.g., to reduce public API surface), feel free to ignore this.
18-18: JoinSet-based concurrency works, but reduce cloning and consider bounding parallelismThe async fan-out via
JoinSetis a good fit here, but there are a few refinements worth considering:
- Avoid cloning
ClangParamsper fileRight now a full
ClangParamsclone is created for every file:for file in files { let arc_params = Arc::new(clang_params.clone()); let arc_file = Arc::clone(file); executors.spawn(analyze_single_file(arc_file, arc_params)); }If
clang_paramscontains a largedatabase_json, this can get expensive. You can clone it once and just clone theArcper task:- let mut executors = JoinSet::new(); - // iterate over the discovered files and run the clang tools - for file in files { - let arc_params = Arc::new(clang_params.clone()); - let arc_file = Arc::clone(file); - executors.spawn(analyze_single_file(arc_file, arc_params)); - } + let shared_params = Arc::new(clang_params.clone()); + let mut executors = JoinSet::new(); + // iterate over the discovered files and run the clang tools + for file in files { + let arc_file = Arc::clone(file); + let arc_params = Arc::clone(&shared_params); + executors.spawn(analyze_single_file(arc_file, arc_params)); + }
- Bound concurrency to avoid spawning too many clang jobs at once
JoinSetwill happily run one task per file; on very large PRs this could translate into a large number of concurrent clang-tidy/clang-format processes and potentially thrash the machine. If that’s a concern, consider enforcing a max concurrency (e.g., with atokio::sync::Semaphoreor by using afor_each_concurrent-style pattern) so you only process N files in parallel.
- More generic parameter type for
filesSince
capture_clang_tools_outputdoesn’t mutatefiles, taking a slice instead of&Vecwould make the API more flexible:-pub async fn capture_clang_tools_output( - files: &Vec<Arc<Mutex<FileObj>>>, +pub async fn capture_clang_tools_output( + files: &[Arc<Mutex<FileObj>>],Given the earlier learning that this function is only called from
run.rs, adjusting the call site should be straightforward. Based on learnings, this keeps the API cleaner while preserving behavior.
- Error handling behavior with
output??Using
output??means the first failing task (either join error or innerResult::Err) will cause the function to return early and drop the remaining tasks in theJoinSet. If that’s intended (fail-fast behavior), it’s fine. If you’d rather attempt to collect diagnostics for all files even when one fails, you’d need to accumulate per-task errors instead of bailing on the first one.Also applies to: 205-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp-linter/src/clang_tools/mod.rs(4 hunks)cpp-linter/src/run.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp-linter/src/run.rs
🧰 Additional context used
🧠 Learnings (5)
📚 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
📚 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
📚 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
📚 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
📚 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
⏰ 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: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: powerpc64le-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-musl
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: Build x86_64-apple-darwin
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build aarch64-apple-darwin
- GitHub Check: Build x86_64-unknown-linux-musl
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: Build x86_64-unknown-linux-gnu
- GitHub Check: Build FreeBSD
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cpp-linter/src/clang_tools/mod.rs (1)
185-185: Remove redundant cloneSince
file_nameis already owned and not used after this return statement, the.clone()is unnecessary.Apply this diff:
- Ok((file_name.clone(), logs)) + Ok((file_name, logs))cpp-linter/tests/reviews.rs (2)
75-85: Env mutations marked unsafe are consistent with new lint expectations but still globally sharedWrapping the
env::set_var/env::remove_varcalls in a singleunsafeblock makes the global state side‑effects explicit and keeps behavior identical to before. Given these tests rely heavily on process‑wide env, this is reasonable.One thing to keep in mind: if the test suite is ever run with multiple test threads, these shared env mutations can still race with other tests that read/write the same vars. If you hit intermittent issues in the future, consider either:
- serializing tests that touch env (e.g., via a serial test attribute or a single integration test driver), or
- introducing a small helper that snapshots and restores relevant env vars around
setup().For now, the changes here look good and align with the pattern used in the other test modules.
Also applies to: 90-92, 98-100
216-221: Verify the intentional divergence betweentidy_ignoreandformat_ignoreunderforce_lgtmPreviously both tools shared a single ignore pattern string; now, when
force_lgtmis true, the sets differ:
format_ignore:**/*.c|**/*.cpp|**/*.htidy_ignore:**/*.c|**/*.hppThat means, for example,
.cppfiles are no longer ignored for tidy, and.hppfiles are no longer ignored for format, which is a behavioral change from a single shared ignore list.If this asymmetry is intentional to match how the test assets are laid out per tool, it would be good to add a short comment here explaining why the ignore sets differ to avoid future “cleanup” regressions.
If instead the goal was to preserve the previous semantics (same ignore patterns for both tools, just passed to two different flags), you may want to push the same extensions to both
format_ignoreandtidy_ignore.Also applies to: 227-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp-linter/src/clang_tools/mod.rs(9 hunks)cpp-linter/tests/reviews.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/tests/reviews.rscpp-linter/src/clang_tools/mod.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/tests/reviews.rscpp-linter/src/clang_tools/mod.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/tests/reviews.rscpp-linter/src/clang_tools/mod.rs
📚 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
📚 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
🧬 Code graph analysis (1)
cpp-linter/tests/reviews.rs (1)
cpp-linter/tests/common/mod.rs (1)
mock_server(82-84)
⏰ 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: build-mkdocs
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: Build FreeBSD
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: x86_64-apple-darwin
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: aarch64-apple-darwin
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: powerpc64le-unknown-linux-gnu
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: x86_64-unknown-linux-musl
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
🔇 Additional comments (11)
cpp-linter/src/clang_tools/mod.rs (11)
16-16: LGTM: Tokio imports for async operationsThe added imports (
fs,process::Command,task::JoinSet) are appropriate for the async conversion of clang tool execution and file I/O.
116-125: LGTM: Async version captureThe conversion to async using
tokio::Commandis correct. The function properly awaits the command output and error handling remains intact.
141-146: LGTM: Safe file name extraction patternThe mutex locking pattern is correct: lock the FileObj, clone the name, and immediately drop the lock. This avoids holding the lock during expensive async operations.
154-154: LGTM: Async clang-format executionThe function properly awaits the async
run_clang_formatcall.
173-173: LGTM: Async clang-tidy executionThe function properly awaits the async
run_clang_tidycall.
213-213: LGTM: Awaiting version capturesBoth
capture_versioncalls are properly awaited and error handling is preserved.Also applies to: 223-223
234-234: LGTM: Async file readThe conversion to
tokio::fs::readwith.awaitis correct for async I/O operations.
248-248: LGTM: Async task spawningThe task spawning pattern correctly uses
JoinSetto manage concurrent file analysis, with proper Arc cloning for shared ownership.
251-258: LGTM: Task result processingThe task joining and result processing is correct. The
output??pattern on line 252 properly handles both theJoinHandleresult and the task'sResulttype. The log grouping per file is well-structured.
202-207: Call site inrun.rscorrectly updated for signature changeThe signature change from
&mut Vec<FileObj>to&Vec<Arc<Mutex<FileObj>>>is properly reflected at the call site incpp-linter/src/run.rs:140-145. Thearc_filesvariable (constructed fromArc::new(Mutex::new(file))at line 133) is passed as&arc_files, providing the correct type match for the new signature.
189-189: Debug derive removal is safe; no code relies on it.Verification confirms that
ClangVersionsis not used with theDebugtrait anywhere in the codebase. All usages involve field access (format_version,tidy_version) orDefaulttrait initialization. The removal does not break existing code.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #224 +/- ##
==========================================
+ Coverage 96.93% 97.08% +0.14%
==========================================
Files 14 14
Lines 3033 3051 +18
==========================================
+ Hits 2940 2962 +22
+ Misses 93 89 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cpp-linter/src/clang_tools/clang_tidy.rs (2)
235-249: Arc<Mutex> adaptation intally_tidy_adviceis fine; consider non-panicking locksUsing
&[Arc<Mutex<FileObj>>]aligns this helper with the rest of the async pipeline. The only nit isfile.lock().unwrap(), which can still panic on a poisoned mutex; you might eventually want to mirror the error-mapping pattern you use inrun_clang_tidyto keep this path infallible and report an error instead of panicking.
251-351: Asyncrun_clang_tidylooks correct; a couple of optional refinements
- Good: you only hold the
Mutex<FileObj>long enough to computefile_name, ranges, and--line-filter, and the lock is released before any.await, avoiding blocking the async scheduler on a sync mutex.- Good: the
tidy_reviewpath cleanly caches the patched content and restores the original file contents afterward, and the advice is written back under a short lock scope at the end.- Optional:
fs::read_to_string,fs::read, andfs::writeare still blocking; if this ever becomes a throughput bottleneck on large repos, consider migrating these specific calls totokio::fsto avoid tying up worker threads.- Optional:
args.extend(["--extra-arg".to_string(), format!("\"{}\"", arg)]);means the quotes are part of the actual argument passed toclang-tidy, not just for display. If the intent is only to show shell-style quoting in the logs, you could keep the rawargin theCommandand apply quoting only when building the"Running ..."log string.cpp-linter/src/clang_tools/clang_format.rs (2)
69-81:tally_format_adviceArc<Mutex> change is fine; lock unwrap is a minor nitSwitching to
&[Arc<Mutex<FileObj>>]keeps tallying aligned with the new shared-ownership model. As with the tidy tally, the use offile.lock().unwrap()can panic on a poisoned mutex; if you ever want this path to be fully robust, you could propagate or log the lock error instead of panicking.
83-191: Asyncrun_clang_formatis structured well; consider a couple of small cleanups
- Good: you gather
file_name, style, and line ranges under a short-lived lock, then drop the lock before any.await, so no syncMutexis held across async boundaries.- Good: the
format_reviewpath obtains a “patched” buffer in oneclang-formatinvocation, while a second invocation with--output-replacements-xmlis used for structured replacement data, both viatokio::process::Command.- Good: when
stdoutis empty you fall back toFormatAdvice::default(), and otherwise you surface XML/UTF‑8 issues viawith_context, which matches the expectation that tool-generated XML should generally parse cleanly.- Optional: the doc comment above this function still says “Run clang-tidy...”; you might want to update it to “Run clang-format...” for clarity.
- Optional: as in
run_clang_tidy, thefs::readused when translating offsets to line numbers is blocking; if format review becomes a hot path, you could eventually switch this totokio::fs::read.cpp-linter/src/clang_tools/mod.rs (1)
116-126: Asynccapture_versionis straightforward; consider adding context on failureUsing
tokio::process::Commandto run<tool> --versionand a regex to extract the version keeps behavior consistent with the prior sync implementation. If you want slightly better diagnosability, you could wrap the.output().await?call inwith_contextto surface which tool path failed when the process execution itself errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/clang_tools/clang_format.rs(6 hunks)cpp-linter/src/clang_tools/clang_tidy.rs(7 hunks)cpp-linter/src/clang_tools/mod.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp-linter/Cargo.toml
🧰 Additional context used
🧠 Learnings (5)
📚 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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.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/clang_tidy.rscpp-linter/src/clang_tools/mod.rs
📚 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/clang_tidy.rscpp-linter/src/clang_tools/mod.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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.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
🧬 Code graph analysis (2)
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)
cpp-linter/src/clang_tools/clang_format.rs (1)
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). (19)
- GitHub Check: build-mkdocs
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: aarch64-apple-darwin
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build i686-pc-windows-msvc
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: Build aarch64-apple-darwin
- GitHub Check: Build aarch64-pc-windows-msvc
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build aarch64-unknown-linux-gnu
- GitHub Check: Build x86_64-unknown-linux-musl
- GitHub Check: Build x86_64-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-musl
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: Build x86_64-apple-darwin
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: Build FreeBSD
🔇 Additional comments (5)
cpp-linter/src/clang_tools/clang_tidy.rs (1)
432-480: Tokio-baseduse_extra_argstest matches the new async APIThe conversion to
#[tokio::test]andArc<Mutex<FileObj>>matches the updatedrun_clang_tidysignature, and the test still verifies thatextra_argsshow up in the logged command string, which is the behavior this path is responsible for.cpp-linter/src/clang_tools/clang_format.rs (1)
21-28:FormatAdvicedefaults + XML no-replacement test look solidDeriving
DefaultforFormatAdviceand adding#[serde(rename(deserialize = "replacement"), default)]toreplacementspairs nicely with the newparse_xml_no_replacementstest:<replacements>elements with no children now deserialize toFormatAdvice { replacements: vec![], patched: None }, which matches the intended “no fixes” semantics without treating it as a parse error. This is consistent with the prior design of only treating truly blank XML as the exceptional case. Based on learnings.Also applies to: 204-218
cpp-linter/src/clang_tools/mod.rs (3)
137-185:analyze_single_fileasync flow and locking strategy look good
- You grab
file_nameunder a short lock, then drop the lock before any calls torun_clang_format/run_clang_tidy, so no synchronous mutex is held across awaits.- The filter checks use
is_none_or(|f| f.is_source_or_ignored(file_name.as_path())), which keeps the logic clear: only run a tool when it’s enabled and not explicitly ignored for this file.- Logs from each tool invocation are simply appended to the local
logsvec and returned alongside thefile_name, which meshes cleanly with the grouping done incapture_clang_tools_output.
188-195:ClangVersionsnow derivingDefaultsimplifies callersDeriving
DefaultonClangVersionsletscapture_clang_tools_outputand summarization code construct an “all tools unused” state without boilerplate, while still allowing each field to be filled only when its corresponding tool actually runs.
201-262:capture_clang_tools_output’s async orchestration and fast-fail behavior look solid
- Version discovery now awaits the async
capture_versionfor each enabled tool, with versions stored inClangVersionsand the resolved executables wired back intoclang_paramsbefore any file analysis.- Cloning
clang_paramsinto anArcand spawning oneanalyze_single_filetask per file viaJoinSetgives you per-file concurrency without sharing mutable state across tasks.- The
while let Some(output) = executors.join_next().await { let (file_name, logs) = output??; ... }pattern correctly fast-fails on both spawn (JoinError) and per-file analysis errors (inneranyhow::Error), aborting remaining tasks when an error is encountered, as described in your comment.- Grouping each file’s logs between
start_log_group("Analyzing <path>")andend_log_group()keeps the REST API output structured even when files complete out of order under concurrency.
|
There is roughly about a 10 second increase in the benchmark with this patch. I'm not sure if this is worth merging.
Command named "rust-previous" is the base branch for this PR. And "rust" is the head commit for this PR. I'm going to abstract certain changes into a separate branch and try to isolate what causes the performance regression. |
2bndy5
force-pushed
the
tokio-command
branch
from
ff739ad to
634173c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/clang_tools/clang_format.rs(4 hunks)cpp-linter/src/clang_tools/clang_tidy.rs(7 hunks)cpp-linter/src/clang_tools/mod.rs(8 hunks)cpp-linter/src/run.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp-linter/src/run.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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: 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/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.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/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.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/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rs
📚 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/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.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
🧬 Code graph analysis (3)
cpp-linter/src/clang_tools/clang_format.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)
cpp-linter/src/clang_tools/clang_tidy.rs (1)
cpp-linter/src/common_fs/mod.rs (1)
new(44-53)
cpp-linter/src/clang_tools/mod.rs (6)
cpp-linter/src/common_fs/file_filter.rs (1)
new(17-24)cpp-linter/src/common_fs/mod.rs (1)
new(44-53)cpp-linter/src/rest_api/github/specific_api.rs (1)
new(33-80)cpp-linter/src/logger.rs (1)
log(32-65)cpp-linter/src/clang_tools/clang_tidy.rs (3)
logs(468-472)run_clang_tidy(250-348)run_clang_tidy(456-467)cpp-linter/src/clang_tools/clang_format.rs (1)
run_clang_format(84-191)
⏰ 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). (19)
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: x86_64-apple-darwin
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: Build i686-pc-windows-msvc
- 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 armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-unknown-linux-gnu
- GitHub Check: Build aarch64-pc-windows-msvc
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: Build current binary
- GitHub Check: Build FreeBSD
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (14)
cpp-linter/Cargo.toml (1)
31-31: LGTM! Tokio process feature correctly enabled.The addition of the "process" feature is necessary to support the async process execution using
tokio::process::Commandintroduced in the clang tool modules.cpp-linter/src/clang_tools/clang_tidy.rs (5)
250-253: Function signature updated to async pattern.The conversion to async and the use of
&Arc<Mutex<FileObj>>enables concurrent file processing across multiple tasks.
300-313: Command execution properly awaited.The async command execution and error handling look correct.
341-346: Result storage with proper error handling.The mutex lock for storing results uses
map_errto provide context on lock failures, which is appropriate.
429-477: Test properly converted to async.The test now uses
#[tokio::test]and awaits the async function call correctly.
255-283: The initialto_string_lossy().to_string()allocation is necessary for ownership, but unnecessary clones occur in error messages.The mutex is correctly scoped, but the concern about string allocations is partially valid. The
file_nameString must be owned to be moved out of the locked scope and passed toCommand. However, unnecessary additional clones occur at lines 291 and 315 in error message formatting contexts where&file_namewould suffice. Consider optimizing by using references in theformat!()macros rather than cloning:format!("Failed to cache file's original content before applying clang-tidy changes: {}", &file_name)This same pattern repeats across 9 locations in the codebase (clang_format.rs, git.rs, file_filter.rs, etc.), so addressing it could yield measurable improvements.
cpp-linter/src/clang_tools/clang_format.rs (3)
21-28: FormatAdvice correctly derives Default.The addition of
Defaultderive and thedefaultattribute on thereplacementsfield enables proper deserialization of XML responses with no replacements.Based on learnings, clang-format XML parsing falls back to empty results when appropriate.
84-102: Mutex lock properly scoped for argument preparation.The file data is extracted within a minimal lock scope, which is good for concurrency.
184-189: Result storage properly handles mutex locking.The error context on mutex lock failure is appropriate.
cpp-linter/src/clang_tools/mod.rs (5)
17-17: Async imports correctly added.The addition of
tokio::process::Commandandtokio::task::JoinSetsupports the async task orchestration pattern.
117-126: Version capture properly converted to async.The async command execution is correct and properly propagates errors.
137-185: File analysis properly converted to async with proper scoping.The function extracts the file name within a minimal mutex lock scope (lines 142-147) and correctly awaits the async tool functions.
188-188: Debug derive added to ClangVersions.This addition is helpful for debugging but doesn't affect functionality.
245-260: Task spawning and result collection properly implemented.The JoinSet usage for concurrent file processing is correct, and the error handling with
output??provides proper fast-fail behavior.
| let mut cmd = Command::new(program); | ||
| cmd.args(&args); | ||
| let patched = if !clang_params.format_review { | ||
| None | ||
| } else { | ||
| logs.push(( | ||
| Level::Info, | ||
| format!( | ||
| "Getting format fixes with \"{} {}\"", | ||
| clang_params | ||
| .clang_format_command | ||
| .as_ref() | ||
| .unwrap() | ||
| .to_str() | ||
| .unwrap_or_default(), | ||
| cmd.get_args() | ||
| .map(|a| a.to_string_lossy()) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| "Getting format fixes with \"{} {} {}\"", | ||
| program.to_string_lossy(), | ||
| args.join(" "), | ||
| &file_name | ||
| ), | ||
| )); | ||
| cmd.arg(&file_name); | ||
| Some( | ||
| cmd.output() | ||
| .await | ||
| .with_context(|| format!("Failed to get fixes from clang-format: {file_name}"))? | ||
| .stdout, | ||
| ) | ||
| }; | ||
| cmd.arg("--output-replacements-xml"); | ||
| args.extend(["--output-replacements-xml".to_string(), file_name.clone()]); | ||
| let mut cmd = Command::new(program); | ||
| cmd.args(&args); | ||
| logs.push(( | ||
| log::Level::Info, | ||
| format!( | ||
| "Running \"{} {}\"", | ||
| cmd.get_program().to_string_lossy(), | ||
| cmd.get_args() | ||
| .map(|x| x.to_string_lossy()) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| program.to_string_lossy(), | ||
| args.join(" ") | ||
| ), | ||
| )); | ||
| let output = cmd | ||
| .output() | ||
| .await | ||
| .with_context(|| format!("Failed to get replacements from clang-format: {file_name}"))?; |
There was a problem hiding this comment.
Sequential command execution may contribute to performance regression.
The code creates and executes two separate tokio::process::Command instances sequentially (lines 103-124 for patched content, then 126-139 for XML replacements). Each command spawns a new clang-format process, and the second execution waits for the first to complete.
Given the PR objectives report a ~10s performance regression, this sequential pattern—repeated across all files—could be a significant contributor. Each file now requires:
- First command: get formatted output (if
format_reviewis enabled) - Second command: get XML replacements
Consider these alternatives:
- If both outputs are always needed, explore whether clang-format can provide both in a single invocation
- If not, consider spawning both commands concurrently using
tokio::join!ortokio::try_join! - Profile to determine if the two-process overhead is actually the bottleneck
// Example concurrent approach (if both commands are always needed):
let (patched_result, xml_result) = tokio::try_join!(
async {
// First command
},
async {
// Second command
}
)?;There was a problem hiding this comment.
- clang-format does not offer a mechanism that combines the 2 outputs
- using concurrency to run both tasks would produce a race condition. We need the formatted first before we restore the un-formatted content get get the XML replacements based on the un-formatted content.
In the future, we may just use formatted output and diff that with original content instead of parsing XML output and calculating the changed line numbers from the XML.
There was a problem hiding this comment.
@2bndy5, thank you for the detailed clarification! I understand now—the sequential execution is necessary because the two commands operate on different file states. Since you need the formatted output first, then must restore the unformatted content before obtaining XML replacements based on that original state, concurrent execution would indeed introduce a race condition.
The future plan to use a diff-based approach instead of XML parsing sounds like a cleaner solution. That would eliminate the need for two separate clang-format invocations per file.
I appreciate you explaining this architectural constraint. Since the sequential pattern is intentional and necessary given clang-format's capabilities, the performance regression likely stems from other aspects of the tokio conversion (perhaps the async overhead itself or changes in how file I/O is handled).
✏️ Learnings added
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
addresses #192 This coverts functions that run and capture clang tools' suggestions from blocking to async API. update uv.lock file * remove duplicated condition; ranges is empty if lines_changed_only is Off
2bndy5
force-pushed
the
tokio-command
branch
from
634173c to
f4d0189
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cpp-linter/src/clang_tools/clang_tidy.rs (1)
250-347: Asyncrun_clang_tidyconversion and locking look correctThe async refactor keeps the important invariants:
- Args (including
--line-filterand--extra-argvalues) andfile_nameare built under a short-lived lock, and the lock is released before any.await, so there’s no risk of holding a mutex across an async suspension point.- The
tidy_reviewflow still:
- Caches original file contents before adding
--fix-errors,- Runs clang-tidy once,
- Caches the patched bytes,
- Restores the original contents, and
- Writes
tidy_adviceback under a fresh lock.All of that matches the previous synchronous behaviour in an async‑safe way.
If you continue to chase the performance regression later, one non-blocking improvement to consider (in a separate change) is moving the
fs::read_to_string/fs::writecalls in thetidy_reviewbranch onto a blocking thread (e.g., via a dedicated blocking I/O helper) so large file sets don’t tie up core async worker threads, but that’s not a correctness issue for this PR.cpp-linter/src/clang_tools/clang_format.rs (1)
84-191: Asyncrun_clang_formatkeeps required sequential behaviour and safe locking
- The async
run_clang_formatnow:
- Acquires the
FileObjmutex only to computefile_name, style, and--linesranges, and releases it before any.await.- When
format_reviewis enabled, runs two clang-format invocations sequentially:
- Once to capture the formatted output (
patchedbuffer),- Then again with
--output-replacements-xmlto get XML replacements.- Uses the original file contents to translate byte offsets to line numbers and then filters replacements by the previously computed ranges.
- Writes
format_adviceback under a fresh lock.This preserves the previous invariant that the two clang-format runs operate on the correct file state while remaining async‑safe. Based on learnings, this sequential two-call pattern is intentional and necessary to avoid races between formatted output and XML replacements.
Minor optional polish: the doc comment above
run_clang_formatstill says “Run clang-tidy…”, which you might want to update to “clang-format” to avoid confusion.cpp-linter/src/clang_tools/mod.rs (2)
116-126: Asynccapture_versionis straightforward, but consider adding context on failureThe async
capture_versioncorrectly shells out to--versionand reuses the existing regex extraction logic. For easier debugging when the process invocation itself fails (e.g., missing binary, permissions), you might optionally wrap the.output().await?withwith_contextto attach the tool path to the error, similar to other call sites, but the current behaviour is functionally correct.
137-185: Per-file async orchestration is correct; consider future concurrency tuningThe async orchestration across these pieces looks solid:
analyze_single_fileacquires theFileObjmutex only long enough to copyfile_name, then:
- Applies the format and tidy filters using that
file_name,- Runs
run_clang_formatandrun_clang_tidysequentially for that file (when enabled),- Accumulates per-file logs and returns
(file_name, logs).
capture_clang_tools_output:
- Resolves tool paths and versions once, then populates
clang_params.- Parses the compilation database (if present) once into
database_jsonand shares it.- Wraps
clang_paramsin a singleArc(avoiding repeated deep clones) and spawns oneanalyze_single_filetask per file viaJoinSet.- Uses
while let Some(output) = executors.join_next().await { let (file_name, logs) = output??; ... }so any error from a task or spawn short-circuits and remaining tasks are aborted when theJoinSetis dropped.This preserves prior behaviour while integrating cleanly with the async runtime.
Given the reported performance regression and that each file can now drive up to two external tool processes concurrently, a future tuning option (in a separate change) would be to cap the number of in-flight
analyze_single_filetasks (e.g., via a semaphore or similar) to avoid oversubscribing CPUs on very large repos. Not required for correctness, but worth considering when you profile further.Also applies to: 201-262, 17-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/clang_tools/clang_format.rs(4 hunks)cpp-linter/src/clang_tools/clang_tidy.rs(5 hunks)cpp-linter/src/clang_tools/mod.rs(7 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.
📚 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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.rs
📚 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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.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/clang_tidy.rscpp-linter/src/clang_tools/clang_format.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
🧬 Code graph analysis (2)
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)
cpp-linter/src/clang_tools/clang_format.rs (1)
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). (9)
- GitHub Check: Build i686-pc-windows-msvc
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: Build FreeBSD
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (3)
cpp-linter/Cargo.toml (1)
31-31: Tokioprocessfeature matches new async process usageEnabling the
processfeature ontokiois consistent with the newtokio::process::Commandusage and looks correct.cpp-linter/src/clang_tools/clang_tidy.rs (1)
429-477: Async test still validates extra args as intendedThe
use_extra_argstest’s conversion to an async test correctly awaitsrun_clang_tidyand still asserts that the logged command line contains the quotedextra_args, so coverage of that behaviour is preserved.cpp-linter/src/clang_tools/clang_format.rs (1)
21-28: Serde defaults forFormatAdvice/Replacementalign with XML testsDeriving
DefaultforFormatAdviceand adding#[serde(default)]to bothreplacementsandReplacement::linecleanly handle the “no<replacement>elements” case and unspecifiedlinevalues, which matches the updated tests that expect an empty/defaultFormatAdvicewhen the XML contains no replacements.Also applies to: 41-53, 204-215
ConclusionThis patch costs too much performance |
addresses #192
This coverts functions that run and capture clang tools' suggestions from blocking to async API.
Other changes
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.