Conversation
Corresponds to cpp-linter/cpp-linter#173. This should never happen in the wild, but just in case there's an underlying problem with parsing clang-tidy output... This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs. Specifically when the diagnostic name does not satisfy the following conditions: - starts with "clang-analyzer-" - starts with "clang-diagnostic-" - contains at least 1 hyphen (`-`) This also removes the `.unwrap()` calls (bad practice) and adds a test for code coverage.
WalkthroughUpdates diagnostic_link in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-04T06:50:10.870ZApplied to files:
📚 Learning: 2025-01-21T09:56:32.771ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
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]](proxy.php?url=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/clang_tidy.rs (1)
77-79: Consider adding a clarifying comment for the special case.The early return for "clang-diagnostic-*" diagnostics is correct, but it would help future maintainers to understand why these diagnostics are treated differently (i.e., they don't have dedicated documentation pages).
🔎 Suggested comment addition
pub fn diagnostic_link(&self) -> String { + // clang-diagnostic-* diagnostics are compiler diagnostics and don't have + // dedicated clang-tidy documentation pages, so return the name as-is. if self.diagnostic.starts_with("clang-diagnostic-") { return self.diagnostic.clone(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp-linter/src/clang_tools/clang_tidy.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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.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.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: build-mkdocs
- GitHub Check: Build x86_64-unknown-linux-gnu
- GitHub Check: Build i686-pc-windows-msvc
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-apple-darwin
- GitHub Check: Build aarch64-unknown-linux-gnu
- GitHub Check: Build x86_64-unknown-linux-musl
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build aarch64-pc-windows-msvc
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: aarch64-unknown-linux-gnu
- GitHub Check: Build FreeBSD
- GitHub Check: test (ubuntu-latest)
- GitHub Check: Build current binary
- GitHub Check: test (windows-latest)
🔇 Additional comments (2)
cpp-linter/src/clang_tools/clang_tidy.rs (2)
400-414: LGTM! Test validates the fallback behavior.The test correctly verifies that diagnostics without recognized patterns (no hyphen, not starting with "clang-analyzer-" or "clang-diagnostic-") return the diagnostic name as-is. Combined with existing tests, this provides good coverage of the diagnostic_link logic.
80-96: The refactored logic correctly handles all diagnostic patterns and generates valid documentation links.The implementation properly:
- Extracts category and name for "clang-analyzer-*" diagnostics by stripping the prefix
- Splits other diagnostics on the first hyphen for link construction
- Falls back to returning the diagnostic name when no pattern matches
The URL format
https://clang.llvm.org/extra/clang-tidy/checks/{category}/{name}.htmlmatches the actual clang-tidy documentation structure. Thedebug_assert!is acceptable given that clang-tidy doesn't produce diagnostics with empty categories or names.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 96.93% 96.94% +0.01%
==========================================
Files 14 14
Lines 3033 3048 +15
==========================================
+ Hits 2940 2955 +15
Misses 93 93 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Corresponds to cpp-linter/cpp-linter#173. This should never happen in the wild, but just in case there's an underlying problem with parsing clang-tidy output...
This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs.
Specifically when the diagnostic name does not satisfy the following conditions:
-)This also removes the
.unwrap()calls (bad practice) and adds a test for code coverage.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.