Skip to content

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Oct 2, 2025

corresponds to cpp-linter/cpp-linter#162

Summary by CodeRabbit

  • New Features
    • Improved compatibility with extended clang-tidy headers, ensuring notes are recognized even with additional suffixes (e.g., warnings-as-errors).
  • Tests
    • Updated demo configuration to treat all warnings as errors; aligned tests with new header handling.
  • Chores
    • CI steps for installing and collecting coverage now run on all operating systems, not just Linux.

@2bndy5 2bndy5 added the bug Something isn't working label Oct 2, 2025
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

Walkthrough

Refactored clang-tidy note header matching to use a named regex constant and updated associated tests. Adjusted demo .clang-tidy to treat all warnings as errors. Modified CI workflow to run specific clang install and coverage steps on all runners by removing Linux-only conditions.

Changes

Cohort / File(s) Summary
Clang-tidy parsing and tests
cpp-linter/src/clang_tools/clang_tidy.rs
Introduced NOTE_HEADER regex constant; replaced inline Regex construction; updated parse_tidy_output to use NOTE_HEADER; adapted inline tests to import NOTE_HEADER and handle extended header formats and capture groups.
Demo clang-tidy config
cpp-linter/tests/demo/.clang-tidy
Changed WarningsAsErrors from empty to * to enable all warnings as errors.
CI workflow adjustments
.github/workflows/run-dev-tests.yml
Removed Linux-only guards for installing clang v19/v20/v21 and their coverage steps; these now run on all OS runners.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the core change of enabling clang‐tidy output parsing when the WarningsAsErrors flag is asserted, which directly matches the introduction of the NOTE_HEADER constant and corresponding test updates. It is concise, specific, and follows conventional commit style without extraneous wording.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch parse-WarningsAsErrors

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

codecov bot commented Oct 2, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   96.90%   96.87%   -0.03%     
==========================================
  Files          14       14              
  Lines        3132     3134       +2     
==========================================
+ Hits         3035     3036       +1     
- Misses         97       98       +1     

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

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

@2bndy5 2bndy5 force-pushed the parse-WarningsAsErrors branch from 0bd262f to 6eee85a Compare October 2, 2025 06:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
cpp-linter/src/clang_tools/clang_tidy.rs (1)

400-420: Add assertion to verify diagnostic name excludes suffix.

The test verifies that the full match includes the suffix, but it doesn't explicitly assert that capture group 6 (the diagnostic name) excludes the suffix. Consider adding an assertion to make this behavior explicit.

Apply this diff to add an explicit assertion:

         assert_eq!(
             cap.get(0).unwrap().as_str(),
             format!(
                 "{}:{}:{}: {}:{}[{},-warnings-as-errors]",
                 cap.get(1).unwrap().as_str(),
                 cap.get(2).unwrap().as_str(),
                 cap.get(3).unwrap().as_str(),
                 cap.get(4).unwrap().as_str(),
                 cap.get(5).unwrap().as_str(),
                 cap.get(6).unwrap().as_str()
             )
             .as_str()
-        )
+        );
+        // Verify that capture group 6 contains only the diagnostic name, not the suffix
+        assert_eq!(
+            cap.get(6).unwrap().as_str(),
+            "modernize-use-trailing-return-type"
+        );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd262f and 6eee85a.

📒 Files selected for processing (3)
  • .github/workflows/run-dev-tests.yml (0 hunks)
  • cpp-linter/src/clang_tools/clang_tidy.rs (4 hunks)
  • cpp-linter/tests/demo/.clang-tidy (1 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/run-dev-tests.yml
⏰ 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: lint-js
  • GitHub Check: aarch64-apple-darwin
  • GitHub Check: Build aarch64-unknown-linux-gnu
  • GitHub Check: powerpc64le-unknown-linux-gnu
  • GitHub Check: x86_64-apple-darwin
  • GitHub Check: powerpc64-unknown-linux-gnu
  • GitHub Check: x86_64-unknown-linux-musl
  • GitHub Check: Build aarch64-pc-windows-msvc
  • GitHub Check: powerpc-unknown-linux-gnu
  • GitHub Check: Build i686-pc-windows-msvc
  • GitHub Check: Build aarch64-apple-darwin
  • GitHub Check: arm-unknown-linux-gnueabi
  • GitHub Check: arm-unknown-linux-gnueabihf
  • GitHub Check: Build x86_64-apple-darwin
  • GitHub Check: Build x86_64-pc-windows-msvc
  • GitHub Check: aarch64-unknown-linux-gnu
  • GitHub Check: aarch64-unknown-linux-musl
  • GitHub Check: Build FreeBSD
  • GitHub Check: test (windows-latest)
  • GitHub Check: test (ubuntu-latest)
🔇 Additional comments (3)
cpp-linter/tests/demo/.clang-tidy (1)

3-3: LGTM!

The change to enable all warnings as errors (WarningsAsErrors: '*') is appropriate for testing the clang-tidy output parser's ability to handle the ,-warnings-as-errors suffix in diagnostic headers.

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

144-144: LGTM!

Refactoring to use the NOTE_HEADER constant improves maintainability and makes the regex pattern easier to test and modify.


133-134: Regex handles both old and new formats as expected.

@2bndy5 2bndy5 merged commit 02c500c into main Oct 2, 2025
62 checks passed
@2bndy5 2bndy5 deleted the parse-WarningsAsErrors branch October 2, 2025 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant