feat: use tokio::Command to capture suggestions
#224
Conversation
addresses #192 This coverts functions that run and capture clang tools' suggestions from blocking to async API.
satisfies newer lint rules
added a test too
WalkthroughThe PR converts clang tool execution from synchronous to async (tokio), changes file handling to 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
addresses #192
This coverts functions that run and capture clang tools' suggestions from blocking to async API.
Other changes
Summary by CodeRabbit
Performance Improvements
Dependencies
User-facing Behavior
Testing & Reliability
✏️ Tip: You can customize this high-level summary in your review settings.