fix: remediate remaining clang-tidy violations + Allure integration (batch 7)#232
Open
fix: remediate remaining clang-tidy violations + Allure integration (batch 7)#232
Conversation
- Lower quality-gate baseline from 1783 to 384 (78.5% reduction) - Fix hicpp-use-equals-delete: move deleted members to public in 3 headers - Fix hicpp-signed-bitwise: use unsigned shift counts in types.h - Fix misc-const-correctness: add const to ~50 variables across source files - Fix misc-include-cleaner: remove unused includes, add NOLINT suppression for platform::Mutex/sleep_for from thread.h abstraction layer - Fix bugprone-implicit-widening-of-multiplication-result in net_impl.h - Fix performance-inefficient-vector-operation: add reserve() in 3 files - Fix hicpp-move-const-arg: remove std::move from trivially-copyable types - Fix modernize-use-auto and modernize-use-emplace in sd_message/tcp_transport - Add clang-tidy to Allure reporting via JUnit XML conversion script
Contributor
📝 WalkthroughWalkthroughAdds CI steps to convert clang-tidy text reports into Allure-compatible JUnit XML and upload them; updates clang-tidy baseline. Also applies widespread local const-correctness, small API-declaration relocations, minor casting/bit-packing fixes, unused-include removals, vector preallocation, and small move-semantics and emplace changes across source files. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions (clang-tidy job)
participant Report as clang-tidy report (`build/clang-tidy-report.txt`)
participant Script as scripts/clang_tidy_to_allure.py
participant Artifacts as GitHub Actions artifact storage (allure-results-clang-tidy)
CI->>Report: run clang-tidy and upload raw report
CI->>Script: run converter (reads Report, optional threshold from baseline)
Script->>Script: parse warnings, group by check, build JUnit XML
Script->>CI: write `allure-results/clang-tidy-results.xml`
CI->>Artifacts: upload `allure-results/` as `allure-results-clang-tidy`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/clang_tidy_to_allure.py`:
- Line 93: The testcase name f-string currently omits "threshold=0" due to
truthiness check; change the conditional to explicitly check threshold is not
None so threshold=0 is preserved—update the line building
name=f"violations={total}" + (f" threshold={threshold}" if threshold is not None
else "") (referencing the variables threshold and total used when constructing
the testcase name).
- Line 27: The file is opening the clang-tidy report with locale-dependent
decoding; update the open(report_path) call to use an explicit encoding (e.g.,
encoding="utf-8") so the file is read deterministically (optionally add
errors="replace" or errors="surrogateescape" if you want to tolerate invalid
bytes). Locate the open(report_path) usage in the script (the with
open(report_path) as f: line) and modify it to explicitly pass the encoding
parameter to ensure consistent decoding across CI runners.
- Line 17: Remove the unused import statement "import sys" from the top of the
module (the stray import sys line) so the file no longer references sys; simply
delete that import line to satisfy pre-commit/flake checks and keep the module
tidy.
- Around line 29-31: The current re.match that assigns m can miscapture
bracketed text inside the diagnostic message; update the regex used in the
re.match call so it anchors to the start and end of the line and explicitly
captures the final check token in square brackets (e.g., change the pattern to
require the trailing `[...]` at the end and use a character-class like `[^\]]+`
for that capture, while allowing the message capture to span internal brackets),
so the match reliably extracts filename, line, message, and the final check name
when called as m = re.match(..., line.strip()).
In `@src/events/event_subscriber.cpp`:
- Around line 182-183: The stored callback key and the lookup key are
inconsistent: callbacks are saved with make_field_key(service_id, instance_id,
event_id) but later looked up with make_field_key(service_id, 0, event_id), so
non-zero instance_id entries are never found. Fix by making key generation
consistent—either change the store to use make_field_key(service_id, 0,
event_id) when inserting into field_requests_ or change the lookup to use the
actual instance_id (make_field_key(service_id, instance_id, event_id)); update
the code paths that call or search field_requests_ (references to
field_requests_, make_field_key, service_id, instance_id, event_id) so both
insertion and lookup use the same key scheme.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcc19d2f-80c6-4b55-b1c4-b8503334c352
📒 Files selected for processing (21)
.github/workflows/clang-tidy.ymlclang-tidy-baseline.txtinclude/core/session_manager.hinclude/e2e/e2e_profile_registry.hinclude/platform/posix/net_impl.hinclude/someip/types.hinclude/transport/udp_transport.hscripts/clang_tidy_to_allure.pysrc/e2e/e2e_crc.cppsrc/e2e/e2e_profile_registry.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/someip/message.cppsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpp
💤 Files with no reviewable changes (1)
- src/e2e/e2e_profile_registry.cpp
Comment on lines
+29
to
+31
| m = re.match( | ||
| r"(.+?):(\d+):\d+: warning: (.+?) \[(.+?)\]", line.strip() | ||
| ) |
Contributor
There was a problem hiding this comment.
Harden warning regex to parse the trailing check token reliably.
Lines 29-31 can misparse messages that contain bracketed content inside the diagnostic text. Anchor to full line and capture the final [check] token.
Suggested fix
- m = re.match(
- r"(.+?):(\d+):\d+: warning: (.+?) \[(.+?)\]", line.strip()
- )
+ m = re.match(
+ r"^(.*):(\d+):\d+: warning: (.*) \[([^\]]+)\]$",
+ line.rstrip("\n"),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| m = re.match( | |
| r"(.+?):(\d+):\d+: warning: (.+?) \[(.+?)\]", line.strip() | |
| ) | |
| m = re.match( | |
| r"^(.*):(\d+):\d+: warning: (.*) \[([^\]]+)\]$", | |
| line.rstrip("\n"), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/clang_tidy_to_allure.py` around lines 29 - 31, The current re.match
that assigns m can miscapture bracketed text inside the diagnostic message;
update the regex used in the re.match call so it anchors to the start and end of
the line and explicitly captures the final check token in square brackets (e.g.,
change the pattern to require the trailing `[...]` at the end and use a
character-class like `[^\]]+` for that capture, while allowing the message
capture to span internal brackets), so the match reliably extracts filename,
line, message, and the final check name when called as m = re.match(...,
line.strip()).
Comment on lines
+182
to
183
| const std::string key = make_field_key(service_id, instance_id, event_id); | ||
| field_requests_[key] = std::move(callback); |
Contributor
There was a problem hiding this comment.
Field-response callback key is inconsistent with lookup key.
At Line 183 the callback is stored with make_field_key(service_id, instance_id, event_id), but lookup uses make_field_key(service_id, 0, event_id) (Line 303). For non-zero instance_id, callbacks won’t be found.
💡 Proposed fix
- const std::string key = make_field_key(service_id, instance_id, event_id);
+ // Response matching currently does not carry instance_id; keep keying consistent.
+ const std::string key = make_field_key(service_id, 0, event_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const std::string key = make_field_key(service_id, instance_id, event_id); | |
| field_requests_[key] = std::move(callback); | |
| // Response matching currently does not carry instance_id; keep keying consistent. | |
| const std::string key = make_field_key(service_id, 0, event_id); | |
| field_requests_[key] = std::move(callback); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/events/event_subscriber.cpp` around lines 182 - 183, The stored callback
key and the lookup key are inconsistent: callbacks are saved with
make_field_key(service_id, instance_id, event_id) but later looked up with
make_field_key(service_id, 0, event_id), so non-zero instance_id entries are
never found. Fix by making key generation consistent—either change the store to
use make_field_key(service_id, 0, event_id) when inserting into field_requests_
or change the lookup to use the actual instance_id (make_field_key(service_id,
instance_id, event_id)); update the code paths that call or search
field_requests_ (references to field_requests_, make_field_key, service_id,
instance_id, event_id) so both insertion and lookup use the same key scheme.
Remove unused import sys, collapse multi-line calls per ruff formatter.
Use explicit UTF-8 encoding with error replacement for report parsing. Fix threshold=0 truthiness bug by checking `is not None`.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/clang_tidy_to_allure.py (1)
28-28:⚠️ Potential issue | 🟠 MajorHarden warning parsing to capture the trailing clang-tidy check token correctly.
Line 28 can misparse diagnostics that contain bracketed text in the message body, causing incorrect
checkattribution.Proposed fix
- m = re.match(r"(.+?):(\d+):\d+: warning: (.+?) \[(.+?)\]", line.strip()) + m = re.match( + r"^(.*):(\d+):\d+: warning: (.*) \[([^\]]+)\]$", + line.rstrip("\n"), + )Run this to verify the current pattern vs the anchored pattern behavior:
#!/bin/bash python - <<'PY' import re current = re.compile(r"(.+?):(\d+):\d+: warning: (.+?) \[(.+?)\]") fixed = re.compile(r"^(.*):(\d+):\d+: warning: (.*) \[([^\]]+)\]$") samples = [ "src/a.cpp:10:3: warning: simple message [modernize-use-auto]", "src/b.cpp:20:7: warning: has [internal] brackets [readability-identifier-naming]", ] for s in samples: c = current.match(s.strip()) f = fixed.match(s.rstrip("\n")) print("LINE:", s) print(" current:", (c.group(3), c.group(4)) if c else None) print(" fixed :", (f.group(3), f.group(4)) if f else None) print() PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/clang_tidy_to_allure.py` at line 28, The current inline regex at the m = re.match(...) call can mis-capture bracketed text inside the message body; replace it with an anchored, bracket-safe pattern so the trailing clang-tidy check token is always captured correctly (use a pattern equivalent to ^(.*):(\d+):\d+: warning: (.*) \[([^\]]+)\]$) and update the match usage in scripts/clang_tidy_to_allure.py (the m = re.match(...) location) to use that pattern (optionally precompile with re.compile for clarity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/clang_tidy_to_allure.py`:
- Line 28: The current inline regex at the m = re.match(...) call can
mis-capture bracketed text inside the message body; replace it with an anchored,
bracket-safe pattern so the trailing clang-tidy check token is always captured
correctly (use a pattern equivalent to ^(.*):(\d+):\d+: warning: (.*)
\[([^\]]+)\]$) and update the match usage in scripts/clang_tidy_to_allure.py
(the m = re.match(...) location) to use that pattern (optionally precompile with
re.compile for clarity).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eeb9f225-bee1-427b-8e2b-ef6b6b3c95c8
📒 Files selected for processing (1)
scripts/clang_tidy_to_allure.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
session_manager.h,e2e_profile_registry.h,udp_transport.h16U) intypes.hconstto unmodified variables across source files<unordered_map>,<iostream>,someip/types.h,e2e_config.h); add NOLINT suppression forplatform::Mutex/sleep_forfrom thread.h abstractionnet_impl.hreserve()before loop push_backstd::movefrom trivially-copyableE2EHeaderscripts/clang_tidy_to_allure.pyconverts clang-tidy report to JUnit XML; CI workflow updated to upload asallure-results-clang-tidyartifactTest plan
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Chores